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

Add --remap-path-prefix=$PWD to make dbg builds more reproducible. #139

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

mfarrugi
Copy link
Collaborator

@mfarrugi mfarrugi commented Oct 20, 2018

This should make debug binaries come out (closer to) identical when built by different users or machines.

See also rust-lang/cargo#5505

PWD was usually something like /home/marco/.cache/bazel/_bazel_marco/5e2375638c8763207dcac6c950a3684d/sandbox/linux-sandbox/14/execroot/, which is a transient path. This should already be cause debugging problems, if there are any.

Edit:
This is visible w/ strings, eg:

>> bazel run -s -c dbg //tools/runfiles:runfiles_test && strings bazel-bin/tools/runfiles/runfiles_test | grep -e $USER -e bazel
/checkout/src/liballoc/raw_vec.rsio_bazel_rules_rust/tools/runfiles/data/sample.txtExample Text!assertion failed: `(left == right)`
/home/marco/.cache/bazel/_bazel_marco/4e38d4394f29c071904de81d6fb3e103/sandbox/linux-sandbox/1/execroot/io_bazel_rules_rust


Tangentially related, we could also remap "external/" to "" to make paths in stacktraces attempt to correspond to the source root, rather than the execroot... but I am not convinced this is a good idea.

eg.

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `427`,
 right: `428`', external/examples/hello_runfiles/hello_runfiles.rs:16:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:207
   3: std::panicking::default_hook
             at libstd/panicking.rs:223
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:402
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:349
   6: hello_runfiles::main
             at external/examples/hello_runfiles/hello_runfiles.rs:16
   7: std::rt::lang_start::{{closure}}
             at /checkout/src/libstd/rt.rs:74
   8: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:306
   9: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
  10: std::rt::lang_start_internal
             at libstd/panicking.rs:285
             at libstd/panic.rs:361
             at libstd/rt.rs:58
  11: std::rt::lang_start
             at /checkout/src/libstd/rt.rs:74
  12: main
  13: __libc_start_main
  14: _start

to

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `427`,
 right: `428`', examples/hello_runfiles/hello_runfiles.rs:16:5
...
   6: hello_runfiles::main
             at examples/hello_runfiles/hello_runfiles.rs:16
...

@acmcarther
Copy link
Collaborator

I wasn't familiar with the problem, looked up the following:

Tracking issue for the feature: rust-lang/rust#41555

Ref from book: https://doc.rust-lang.org/1.25.0/unstable-book/compiler-flags/remap-path-prefix.html
I expect that link to break at some point, so the relevant snippets (to me):

The -Z remap-path-prefix-from, -Z remap-path-prefix-to commandline option pair allows
to replace prefixes of any file paths the compiler emits in various places. This is useful
for bringing debuginfo paths into a well-known form and for achieving reproducible builds
independent of the directory the compiler was executed in. All paths emitted by the
compiler are affected, including those in error messages.

The book goes on to describe some reasons why this flag would be set, including a situation where debuggers might want to look at another directory. In both the old impl and with this change, those usecases would not be served since the current directories are ephemeral, and the new directories are lies.

I feel like we could do better for non-external code by linking to the actual workspace. Do you think this is feasible instead of what we're doing here? Or perhaps I've misunderstood this change?

@acmcarther
Copy link
Collaborator

Ah, there is a totally separate problem of making builds reproducible across machines (which appears to have motivated this change).

I would prioritize that over better debugger support. If you can speak to both this comment and the last comment, that would be helpful.

@mfarrugi
Copy link
Collaborator Author

mfarrugi commented Oct 20, 2018 via email

@mfarrugi
Copy link
Collaborator Author

Yes this was 100% geared towards reproducibility.

Copy link
Collaborator

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W.R.T. finding the workspace path. I would expect there to be a lib function that does that, and I did find a stack overflow link.

I think this is a moot point though, as AFAICT the reproducible objective is at odds with the workspace directory objective, and I would prioritize the former.

@mfarrugi mfarrugi merged commit 6584e8a into bazelbuild:master Oct 20, 2018
@mfarrugi mfarrugi deleted the marco-remap-paths branch October 19, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants