Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[WIP, RFC] Use relative paths for __FILE__ #1861

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f990e97
Strip `-fdebug-prefix-map` from compiler flags
jakirkham Mar 14, 2025
96c5811
Map `__FILE__` to a relative path
jakirkham Mar 14, 2025
e2f1daf
Fix `script/content` format
jakirkham Mar 15, 2025
4f67028
Merge branch 'branch-25.04' into use_rel_pth_prefix
bdice Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions conda/recipes/rmm/recipe.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,23 @@ source:
build:
string: cuda${{ cuda_major }}_py${{ py_buildstring }}_${{ date_string }}_${{ head_rev }}
script:
content:
- ./build.sh -v clean rmm --cmake-args=\"-DCMAKE_INSTALL_LIBDIR=lib\"
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')
Copy link
Contributor

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.

Copy link
Member Author

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

set +x
Comment on lines +24 to +30
Copy link
Member Author

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
Comment on lines +32 to +37
Copy link
Member Author

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:

  1. Moving this into our CMake logic for all builds
  2. 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)

Copy link
Contributor

@bdice bdice Mar 17, 2025 •
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Member Author

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


./build.sh -v clean rmm --cmake-args=\"-DCMAKE_INSTALL_LIBDIR=lib\"
secrets:
- AWS_ACCESS_KEY_ID
- AWS_SECRET_ACCESS_KEY
Expand Down