-
Notifications
You must be signed in to change notification settings - Fork 216
[WIP, RFC] Use relative paths for __FILE__
#1861
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
Conversation
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
/ok to test |
The builds completed. Though CI is marked as errored. Not seeing a specific error in the logs. The only thing that sticks out are these warnings:
Are these warnings being treated as errors? |
Ah maybe it is this error during the upload step where the
Did something change in our GHA workflow? Or CI images? |
Looks like this CI error will be fixed by this GHA workflow update: rapidsai/shared-workflows#299 Edit: We need to wait for the nightly job on Edit 2: This is now cleared up. Turns out a new GHA run (instead of restarting) picks up the shared workflow fix |
/ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments below providing more details about the changes
# Remove `-fdebug-prefix-map` line from CFLAGS and CXXFLAGS so the | ||
# incrementing version number in the compile line doesn't break the | ||
# cache | ||
set -x | ||
export CFLAGS=$(echo $CFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') | ||
export CXXFLAGS=$(echo $CXXFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') | ||
set +x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have copied the stripping of the existing -fdebug-prefix-map
over from librmm
:
rmm/conda/recipes/librmm/recipe.yaml
Lines 21 to 28 in f87d7a6
content: | | |
# Remove `-fdebug-prefix-map` line from CFLAGS and CXXFLAGS so the | |
# incrementing version number in the compile line doesn't break the | |
# cache | |
set -x | |
export CFLAGS=$(echo $CFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') | |
export CXXFLAGS=$(echo $CXXFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') | |
set +x |
We would want this in both builds for the same reasons
# Use relative paths for `__FILE__` by passing `-fmacro-prefix-map` | ||
# https://gcc.gnu.org/onlinedocs/gcc-13.3.0/gcc/Preprocessor-Options.html#index-fmacro-prefix-map | ||
set -x | ||
export CFLAGS="${CFLAGS} -fmacro-prefix-map=$(pwd)/=''" | ||
export CXXFLAGS="${CXXFLAGS} -fmacro-prefix-map=$(pwd)/=''" | ||
set +x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As $(pwd)
is the top-level directory of this repo, passing the -fmacro-prefix-map
option simply removes the $(pwd)
portion of the path in all __FILE__
expansions. Thus leaving us with paths relative to the top of the repo
A couple things we may want to consider:
- Moving this into our CMake logic for all builds
- Applying this behavior to more prefix mappings.
-ffile-prefix-map
would do this for all prefix-mapping cases GCC handles
Beyond fixing the __FILE__
expansion this gets us closer to reproducible builds. Also it should behave more robustly with caching (though we should test to confirm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. I would consider merging this as-is, and doing (1) or (2) as follow-up work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also support this approach here. I think this is a good idea. I suggest that we try and put this into rapids-cmake so that we get this for free in all our libraries. The main challenge I see with that is identifying the appropriate root path to use. I agree with Bradley that I'm OK with doing that as a follow-up if you want to get this PR merged sooner. I can open a suitable rapids-cmake issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened rapidsai/rapids-cmake#798
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to describe what this would look like in the context of rapids-cmake
: rapidsai/rapids-cmake#798 (comment)
Though am sure it can be improved further. Please feel free to suggest there
Put together an example here: conda/conda-build#1674 (comment) |
This is really nice. I'm 100% supportive of this approach! |
# cache | ||
set -x | ||
export CFLAGS=$(echo $CFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') | ||
export CXXFLAGS=$(echo $CXXFLAGS | sed -E 's@\-fdebug\-prefix\-map[^ ]*@@g') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also export this in CUDAFLAGS I expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Tried to capture a better recommendation here: rapidsai/rapids-cmake#798 (comment)
Please feel free to suggest further improvements
Closing in favor of addressing this in RAPIDS-CMake via PR: rapidsai/rapids-cmake#817 |
Description
Explore using GCC's
-fmacro-prefix-map
to remap__FILE__
from absolute paths to source directory relative paths.Related to issue: rapidsai/build-planning#159
Checklist