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

cmake: fix relative path calculate error #74710

Closed
wants to merge 1 commit into from

Conversation

LingaoM
Copy link
Collaborator

@LingaoM LingaoM commented Jun 22, 2024

Fixes #74647

image

Copy link
Collaborator

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

This removes the use of relative path which was intentionally added in #16368.

cc @marc-hb is it still required ?

@LingaoM
Copy link
Collaborator Author

LingaoM commented Jun 23, 2024

Besides being shorter and more "private", this makes the content of snippets-*.ld files the same no matter who built them and where.

The build directory absolutely is local generated directory, not public for other.

@carlescufi carlescufi requested a review from marc-hb June 25, 2024 09:33
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 27, 2024

The build directory absolutely is local generated directory, not public for other.

No, the build directory is not "100% private": to make sure the build is reproducible you need to compare object files.

Reproducible builds are important for security, for instance they help with "JiaT75" issues. Comparing build directories also helps catching small but harmful differences between CI and developers and between developers.

For more context see #50205, #14593, https://www.cisa.gov/sbom, https://reproducible-builds.org/ etc.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

This removes the use of relative path which was intentionally added in #16368.

This is indeed a logical revert of d1b4da9, so at least the commit message is wrong. It currently says "fix relative path calculate error" but this commit does not fix any "calculate error", instead it reverts to an absolute, non reproducible path.

Thanks @ithinuel for the good catch.

```C
file(RELATIVE_PATH relpath ${ZEPHYR_BASE}/include ${path})
```

The prerequisite path assumed here must be the prefix of ZEPHYR,
but if use symbolic links it will calculate error: like this:
zephyrproject-rtos#74647 (comment)

Fixes: zephyrproject-rtos#74647

Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
@LingaoM LingaoM force-pushed the fix_relative_path branch from 4b80054 to 5035af4 Compare June 27, 2024 01:27
@LingaoM
Copy link
Collaborator Author

LingaoM commented Jun 27, 2024

@marc-hb @ithinuel I've reworked a version that will still use relative paths, but adds support for link paths.

@LingaoM LingaoM requested review from ithinuel and marc-hb June 27, 2024 01:29
@LingaoM LingaoM added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Jun 28, 2024
@marc-hb marc-hb dismissed their stale review June 29, 2024 00:25

stale review

@marc-hb marc-hb removed the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Jun 29, 2024
Comment on lines +1338 to +1339
get_filename_component(ZEPHYR_BASE_REALPATH "${ZEPHYR_BASE}/include" REALPATH)
get_filename_component(PATH_REALPATH "${path}" REALPATH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The command get_filename_component(... REALPATH) has been superseded by file(REAL_PATH ...) cmake.

get_filename_component

Get a specific component of a full filename.

Changed in version 3.20: This command has been superseded by the cmake_path() command, except for REALPATH, which is now offered by file(REAL_PATH),

https://cmake.org/cmake/help/latest/command/get_filename_component.html#command:cmake_path

but please note that file(REAL_PATH ...) is affected by CMP0152., and before you ask, then yes, get_filename_component(... REAL_PATH) suffers the same issue related to symlink resolving, but doesn't have a policy for controlling this (always fixed at OLD behavior).

Which again shows that the use of symlinks can be tricky.

Also, variables intended for local use should be written in lowercase, like path_realpath and zephyr_base_realpath.
And please use another name than zephyr_base_realpath, as it is not the real path of Zephyr you are defining, but a subfolder (include).

@LingaoM LingaoM closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After menuconfig, west build failed.
5 participants