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

feat: make source path prefix configurable #1529

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

Conversation

roman-kashitsyn
Copy link
Contributor

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.

@UebelAndre
Copy link
Collaborator

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.

It's non-deterministic because the sandbox path gets embedded in binaries? Or rlibs?

@roman-kashitsyn
Copy link
Contributor Author

roman-kashitsyn commented Aug 25, 2022

It's non-deterministic because the sandbox path gets embedded in binaries? Or rlibs?

In the final binaries, that's how we caught the non-determinism in the first place: we check determinism on CI by building inside of a docker container on several Linux machines and comparing the resulting binaries.
Some rlibs might also have been different, I don't have the data ATM.
image

@UebelAndre
Copy link
Collaborator

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

@krasimirgg
Copy link
Collaborator

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 strings occuring in them could give us some more hints where the nondeterminacy comes from. We've seen recently some artifacts where the short_path to a thingy outside of the main bazel repository you're working in sometimes starts with .., requiring some care here and there. I wouldn't be surprised if something like this inside rules_rust itself is a source of such nondeterminacies...

@@ -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.
Copy link
Collaborator

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

@goffrie
Copy link
Contributor

goffrie commented Sep 26, 2022

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.

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.

4 participants