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

Use an absolute path to C/C++ compilers for better compatibility #104

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brians-neptune
Copy link

toolchains_llvm only triggers its logic to locate files relative to the compiler wrapper when called with an absolute path, for example. Otherwise it assumes it's being called from the bazel execroot, which is not true in this case.

toolchains_llvm already has similar logic as wheel_builder for mapping paths, which could be used by just calling it with an absolute path instead of using wheel_builder's wrapper, but I think it's easier to use the wheel_builder version instead of trying to detect whether the toolchain has this logic.

[`toolchains_llvm`][1] only triggers its logic to locate files relative
to the compiler wrapper when called with an absolute path, for example.
Otherwise it assumes it's being called from the bazel execroot, which is
not true in this case.

`toolchains_llvm` already has similar logic as `wheel_builder` for
mapping paths, which could be used by just calling it with an absolute
path instead of using `wheel_builder`'s wrapper, but I think it's easier
to use the `wheel_builder` version instead of trying to detect whether
the toolchain has this logic.

[1]: https://github.com/bazel-contrib/toolchains_llvm/blob/1d685a99db5f1e6523c392e7dfaced583062cbc4/toolchain/cc_wrapper.sh.tpl#L37
@brians-neptune
Copy link
Author

I can add an integration test that uses toolchains_llvm if desired. I'm not sure if you want to take on maintaining that compatibility or not, I think it has a fairly low risk of breaking without the existing tests using hermetic_cc_toolchain catching the problem.

@jvolkman
Copy link
Owner

I originally always used absolute paths for everything, but switched to relative paths at some point because I couldn't keep absolute, ephemeral paths from being included in binaries built by zig cc and preventing reproducibility. Maybe that has improved by now, but I'd like to verify before making such a change.

I'm not completely happy with the current approach, and have thought about maybe coming up with a way to make the sdist area look like a bazel execroot, allowing all of the bazel relative paths to "just work". Maybe that would solve the llvm issue as well?

In either case, I'd happily welcome a toolchains_llvm integration test, as everything is fairly zig-centric currently.

@brians-neptune
Copy link
Author

I don't think you can make it look like an execroot in the general case, you might have filename conflicts.

uber/hermetic_cc_toolchain#76 sounds like they intend to strip absolute paths regardless of it being called with an absolute path.

With this PR, I tried finding any absolute paths after bazelisk build --strip=never -c dbg ... in e2e/workspace, and didn't find any. I checked the following files:

  • bazel-bin/external/poetry_lock_repo/_lock/zstandard@0.22.0/site-packages/zstandard.libs/libzstd-30a224eb.so
  • bazel-bin/external/zstd/_objs/zstd/zstd_compress_sequences.o
  • bazel-bin/external/zstd/libzstd.so

I ran strings on each of those files and searched for my username and rules_pycross, and found nothing. Searching for zstd, I see various file paths like external/zstd/lib/common/pool.c.

Sounds good on adding integration tests, I'll push those shortly.

ddeville added a commit to ddeville/rules_pycross that referenced this pull request Sep 26, 2024
ddeville added a commit to ddeville/rules_pycross that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants