Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: namespace generated headers with zephyr/ #63973

Merged
merged 4 commits into from
May 28, 2024

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Oct 16, 2023

Namespaced the generated headers with zephyr/ to prevent potential conflict with other headers.

Introduced a Kconfig (CONFIG_LEGACY_GENERATED_INCLUDE_PATH) to support legacy generated header include paths in the mean time.

Documentation preview

Fixes #60377
Fixes #68035

Automation script:

#!/usr/bin/env python

from pathlib import Path
import re

EXTENSIONS = ("c", "h", "cpp", "rst", "md", "S", "ld", "py", "svg")

HEADERS = [
    "app_version",
    # "autoconf",
    "cmake_intdef",
    "core-isa-dM",
    "devicetree_generated",
    "driver-validation",
    "kobj-types-enum",
    "linker-kobject-prebuilt-data",
    "linker-kobject-prebuilt-priv-stacks",
    "linker-kobject-prebuilt-rodata",
    "mcuboot_version",
    "offsets",
    "otype-to-size",
    "otype-to-str",
    "strerror_table",
    "strsignal_table",
    "syscall_list",
    "version",
    "zsr",
]

HEADERS_H = [hdr + ".h" for hdr in HEADERS]

for p in Path(".").glob("zephyr/**/*"):
    if not p.is_file() or p.suffix and p.suffix[1:] not in EXTENSIONS:
        continue

    # skip git & github folders
    if (str(p).startswith("zephyr/.git")):
        continue

    # skip release docs
    if (str(p).startswith("zephyr/doc/releases")):
        continue

    content = ""
    with open(p, 'r', newline='', errors="surrogateescape") as f:
        for line in f:
            # for lines in sources
            m = re.match(r"^(.*)#include <(.*)>(.*)$", line)
            # for lines in docs
            n = re.match(r"^(.*)include/generated/(.*)\.h(.*)$", line)
            if (m and (m.group(2).startswith("syscalls/") or m.group(2) in HEADERS_H)):
                content += (m.group(1) + "#include <zephyr/" +
                            m.group(2) + ">" + m.group(3) + "\n")
            elif (n and n.group(2) in HEADERS):
                content += (n.group(1) + "include/generated/zephyr/" +
                            n.group(2) + ".h" + n.group(3) + "\n")
            else:
                content += line

    with open(p, "w", newline='') as f:
        f.write(content)

Requirements for Treewide Changes

  • The zephyr repository must apply the ‘treewide’ GitHub label to any issues or pull requests that are treewide changes
  • The person proposing a treewide change must create an RFC issue describing the change, its rationale and impact, etc. before any pull requests related to the change can be merged
  • The project’s Architecture Working Group (WG) must include the issue on the agenda and discuss whether the project will accept or reject the change before any pull requests related to the change can be merged (with escalation to the TSC if consensus is not reached at the WG)
  • The Architecture WG must specify the procedure for merging any PRs associated with each individual treewide change, including any required approvals for pull requests affecting specific subsystems or extra review time requirements
  • The person proposing a treewide change must email [email protected] about the RFC if it is accepted by the Architecture WG before any pull requests related to the change can be merged

@ycsin ycsin requested a review from cfriedt October 16, 2023 09:42
nordicjm
nordicjm previously approved these changes Oct 16, 2023
@ycsin ycsin force-pushed the pr/zephyr_version_h branch 2 times, most recently from d11cd32 to 6f4c80f Compare October 17, 2023 05:15
@ycsin
Copy link
Member Author

ycsin commented Oct 17, 2023

this PR is getting more involved than I initially expected

EDIT:
And CI is complaining about some lines that are not modified in this PR:

Run if [[ ! -s "compliance.xml" ]]; then
Error: See https://www.pylint.org/ for more details

 C03[25](https://github.com/zephyrproject-rtos/zephyr/actions/runs/6542835611/job/17766473397?pr=63973#step:9:26):Unnecessary parens after '=' keyword (superfluous-parens)
File:scripts/kconfig/kconfiglib.py
Line:2931
Column:0
 C0325:Unnecessary parens after '=' keyword (superfluous-parens)
File:scripts/kconfig/kconfiglib.py
Line:4310
Column:0
 C0325:Unnecessary parens after '=' keyword (superfluous-parens)
File:scripts/kconfig/kconfiglib.py
Line:4448
Column:0
Error: Process completed with exit code 1.

node.is_menuconfig = (t0 is _T_MENUCONFIG)

self._write_to_conf = (vis != 0)

self._write_to_conf = (vis != 0)

This is fixed in the last commit

@ycsin ycsin force-pushed the pr/zephyr_version_h branch 3 times, most recently from 4c025dc to d0ab838 Compare October 17, 2023 09:57
@ycsin ycsin changed the title build: namespace the generated version.h build: namespace generated headers Oct 17, 2023
@ycsin ycsin force-pushed the pr/zephyr_version_h branch 2 times, most recently from 89a285d to 5fb8545 Compare October 17, 2023 14:59
@ycsin ycsin changed the title build: namespace generated headers build: namespace generated headers with zephyr/ Oct 18, 2023
@ycsin ycsin marked this pull request as ready for review October 18, 2023 15:52
@zephyrbot zephyrbot removed manifest-sof manifest-percepio DNM This PR should not be merged (Do Not Merge) labels May 24, 2024
@ycsin ycsin marked this pull request as ready for review May 24, 2024 16:48
Comment on lines +114 to +118
message(WARNING "
Warning: CONFIG_LEGACY_GENERATED_INCLUDE_PATH is currently enabled by default
so that user applications can continue to use the legacy include paths for the
generated headers. This Kconfig will be deprecated and eventually removed in
the future releases.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not fond of having such a warning enabled by default. This will be emitted on every build, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... there are other people who look at build warnings? @henrikbrixandersen, you and I should start a club!

I've seen Kconfig and Device tree warnings pretty much every time I build anything more complex than samples/hello_world/... neither of these tools seems to have any -Werror, which in my experience is the only way to make anyone care.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning will be printed only for people who should fix their code ASAP so printing it every time looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the CONFIG_LEGACY_GENERATED_INCLUDE_PATH which is enabled by default now will just make everything passes silently. Most people might not notice that the headers are now relocated to zephyr/ unless they looked at their build directory or the migration guide.

On the other extreme, if we disable CONFIG_LEGACY_GENERATED_INCLUDE_PATH by default, the downstream apps will fail to build immediately, which I don't think is the right way to do in an OS of this scale.

The current implementation IMHO is the middle approach - we don't want to break downstream applications, but at the same time we want to make sure that developers are made aware of the changes, and given enough time to do their migration (as we do ours - some of the PRs aren't merged, west.yml not updated to point at them).

@ycsin
Copy link
Member Author

ycsin commented May 25, 2024

@cfriedt @nordicjm @finikorg @fabiobaltieri @de-nordic @dcpleung @mbolivar-ampere
This PR is now ready (again), could you please take another look?

@gmarull this PR should align with #41543 and consistent with the header migration PR that you have done in the past (~v3.1.0?), could you please review?

Copy link
Collaborator

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to nRF QSPI NOR look ok.

@fabiobaltieri
Copy link
Member

This may use some coordination help from someone from @zephyrproject-rtos/release, plenty of dependencies, plus probably something will pop up after merge as well, worth keeping it in mind.

@ycsin
Copy link
Member Author

ycsin commented May 27, 2024

ping assignee @tejlmand, PTAL, thanks!

@ycsin
Copy link
Member Author

ycsin commented May 28, 2024

ping assignee @tejlmand, PTAL, thanks!

another ping @tejlmand

@carlescufi carlescufi assigned nashif and unassigned tejlmand May 28, 2024
@henrikbrixandersen henrikbrixandersen merged commit 3570408 into zephyrproject-rtos:main May 28, 2024
49 of 57 checks passed
@ycsin
Copy link
Member Author

ycsin commented May 29, 2024

🥳🍀🤞

@ycsin ycsin deleted the pr/zephyr_version_h branch May 29, 2024 09:42
@marc-hb
Copy link
Collaborator

marc-hb commented May 29, 2024

Just FYI https://github.com/thesofproject/sof/actions/runs/9278328176/job/25529164408

FileNotFoundError:  No such file or directory:
  '/zep_workspace/build-lnl/zephyr/include/generated/autoconf.h'

That's because one Python script in SOF extracts values from autoconf.h after the build.

The C compilation was fine and the CONFIG_LEGACY_GENERATED_INCLUDE_PATH warning was printed by CMake as expected.

Just FYI.

besmarsh added a commit to besmarsh/zephyr that referenced this pull request Jul 8, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
nashif pushed a commit that referenced this pull request Jul 11, 2024
PR #63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
AlienSarlak pushed a commit to AlienSarlak/zephyr that referenced this pull request Jul 13, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
CZKikin pushed a commit to nxp-upstream/zephyr that referenced this pull request Jul 18, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
Devansh0210 pushed a commit to Devansh0210/zephyr that referenced this pull request Jul 20, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
Thalley pushed a commit to Thalley/zephyr that referenced this pull request Aug 1, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

Signed-off-by: Ben Marsh <[email protected]>
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this pull request Aug 26, 2024
PR zephyrproject-rtos#63973 namespaced generated headers with zephyr/, including generated
syscall headers.

Since then, some new generated syscall header includes have been added
without the zephyr/ prefix, breaking builds when
CONFIG_LEGACY_GENERATED_INCLUDE_PATH is disabled.

This commit adds the zephyr/ prefix to includes for generated syscall
headers where it has been missed.

(cherry picked from commit 74c871d)

Original-Signed-off-by: Ben Marsh <[email protected]>
GitOrigin-RevId: 74c871d
Change-Id: I8e595ded94f2900d6c4bcc60ac078aa56f80f22d
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5702156
Commit-Queue: Keith Short <[email protected]>
Tested-by: Keith Short <[email protected]>
Reviewed-by: Keith Short <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[RFC] build: namespace generated headers with zephyr/ posix: uname: follow up enhancements