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

Pass --remap-path-prefix to cargo #1118

Closed
wants to merge 1 commit into from

Conversation

tiziano88
Copy link
Collaborator

This is meant to reduce nondeterminism across machines or docker vs host, but it does not seem to fully
solve the problem yet (it does remove some paths from the resulting
binary, so I think it is necessary, but not sufficient).

Ref #865

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by
      Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

scripts/common Outdated
@@ -10,6 +10,9 @@ set -o pipefail
# https://docs.rs/env_logger
export RUST_LOG="${RUST_LOG:-info}"

# Reduce build nondeterminism by remapping paths so that they don't include local segments.
export RUSTFLAGS="--remap-path-prefix=$PWD=/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to take into account any existing value of RUSTFLAGS (same concern about stomping on user-specified values also applies to scripts/run_tests_tsan btw)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, done.

This is meant to reduce nondeterminism across machines or docker vs host, but it does not seem to fully
solve the problem yet (it does remove some paths from the resulting
binary, so I think it is necessary, but not sufficient).

Ref project-oak#865
@tiziano88
Copy link
Collaborator Author

CI is failing and I have no idea what's going on or why it is related to this change. Even locally, it seems that running run_examples with this change causes some background jobs to terminate early (?) or not terminate at all, and the bash scripts get in a weird state.

@daviddrysdale do you have any hint of what may be happening?

https://pantheon.corp.google.com/cloud-build/builds/6be121f1-9042-4fa6-abcb-d6f89e4388d8;step=9?project=oak-ci

@daviddrysdale
Copy link
Contributor

It looks like the cargo cache is lost somehow. The run_examples script does something like:

  • build_example
  • build_server, which includes cargo build oak_loader
  • run_server &, which includes cargo run --package=oak_loader
  • run client app.

It looks like the run_server & step (in the background) is kicking off a full rebuild of the (just-built) oak_loader, and the client starts up and fails in the meanwhile.

@daviddrysdale
Copy link
Contributor

Although the immediate fix will be for the cargo cache issue, we should probably also not rely on luck for inter-process synchronization. In bash-land something like this has worked for me in the past; doing something equivalent in runner would presumably be straightforward.

@jul-sh jul-sh changed the base branch from master to main June 16, 2020 17:22
@tiziano88
Copy link
Collaborator Author

I'm abandoning this for now, we may resurrect it in a different form in runner if necessary.

@tiziano88 tiziano88 closed this Sep 3, 2020
@tiziano88 tiziano88 deleted the tzn_remap_path branch September 3, 2020 09:58
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.

4 participants