-
Notifications
You must be signed in to change notification settings - Fork 440
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
feat: make source path prefix configurable #1529
base: main
Are you sure you want to change the base?
feat: make source path prefix configurable #1529
Conversation
It's non-deterministic because the sandbox path gets embedded in binaries? Or rlibs? |
Would this issue ultimately be fixed by this feature? I don't think users should need to disable sandboxing to achieve deterministic outputs but am not sure what the rules can do if the problem is actually in |
The nondeterminacy is surprising, especially on linux. Having the path prefix seems reasonable, but I'd like to understand better the source of the nondeterminacy, e.g., is it coming from rustc itself or from some interaction in rules_rust. Could you share a toy example that reproduces the non-determinacy manually in some way? Maybe also do you have the means to examine the differences between the binaries, I suspect something like diffing the |
@@ -737,7 +742,7 @@ def construct_arguments( | |||
force_link (bool, optional): Whether to add link flags to the command regardless of `emit`. | |||
stamp (bool, optional): Whether or not workspace status stamping is enabled. For more details see | |||
https://docs.bazel.build/versions/main/user-manual.html#flag--stamp | |||
remap_path_prefix (str, optional): A value used to remap `${pwd}` to. If set to a falsey value, no prefix will be set. | |||
remap_path_prefix (bool): Whether to apply the remap-path-prefix compiler setting. |
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.
Could you preserve the remap_path_prefix (str, optional)
and move the logic that picks it up from _source_path_prefix
to clients; the implicit dependency on specific custom attributes in ctx.attr
here is a thing we'd like to avoid at this layer, if possible. (We haven't been strictly enforcing this so you can see some instances of attributes used around).
I'm not inherently opposed to this PR, but the nondeterminism issue is likely to actually be #1536. I don't see how this PR could help prevent Bazel paths from making it into the output. |
Currently, the rules apply the
--remap-path-prefix=${pwd}=.
substitution when building inside a sandbox.Sometimes this substitution causes non-determinism in our CI setup.
Specifying a fixed path prefix, such as
/source/
, resolves the non-determinism issue (at least on Linux).This change allows configuring the prefix via the
@rules_rust//:source_path_prefex
option, leaving the current behavior as default.