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

Automate vendoring by invoking cargo-vendor when building src dist tarballs. #39728

Merged
merged 2 commits into from
Feb 14, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 10, 2017

This avoids #39633 bringing the src/vendor checked into git by #37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run cargo vendor during the dist src step is sound.

However, the only way to be sure cargo-vendor exists is to run cargo install --force cargo-vendor, which will recompile it every time (not passing --force means you can't tell between "already exists" and "build error"). This is quite suboptimal and I'd like to somehow do it in each Dockerfile that would need it.

  • Cache CARGO_HOME (i.e. ~/.cargo) between CI runs
    • bin/cargo-vendor and the actual caches are the relevant bits
  • Do not build cargo-vendor all the time
    • Maybe detect ~/.cargo/bin/cargo-vendor already exists?
    • Could also try to build it in a Dockerfile but do we have cargo/rustc there?
    • Final solution: check cargo install --list for a line starting with cargo-vendor

cc @rust-lang/tools

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

src/ci/run.sh Outdated
@@ -51,7 +51,7 @@ fi
# all test suites have all their deps in there (just the main bootstrap) so we
# have the ability to disable this flag
if [ "$NO_VENDOR" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-vendor"
# RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-vendor"
Copy link
Member

Choose a reason for hiding this comment

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

why comment this out rather than delete all this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I'm not sure yet what's supposed to happen. I guess I should remove it if we want to cache .cargo instead through some other means.

@eddyb eddyb force-pushed the vendeur-tres-bien branch 2 times, most recently from 69b92e0 to 38b67b7 Compare February 10, 2017 23:28
@alexcrichton
Copy link
Member

Thanks for taking the time to make a PR @eddyb!

Taking a look at this, here's my thoughts:

  • We'll still want to test --enable-vendor I believe. The best opportunity for this is the "distcheck" bot. Can you update the distcheck step to unconditionally pass --enable-vendor?
  • I think we'll still want the license check in tidy, but we can skip it in most cases unless --enable-vendor is passed. That way we'll run this on the distcheck bot but nowhere else.
  • Another purpose of --enable-vendor is to verify that Cargo.lock in-tree is up-to-date (e.g. doesn't need changes to make it correct). I believe that if we only run --enable-vendor on distcheck we won't catch mistakes. Can you add a new configure flag to pass --locked to Cargo on the bots? We still shouldn't pass the flag by default, only on Travis/AppVeyor.

Other than that I think the biggest question is where does the cargo-vendor binary itself come from. Using cargo install just before can be annoying as it unconditionally compiles the project which currently takes quite awhile (depends on Cargo as a library). I don't think that many users are running this command, though, so this is likely just a pain that we can stomach.

That being said I think we could improve the logic here with:

  • Don't install --force if cargo-vendor is somewhere in PATH. There should be example code in bootstrap/sanity.rs of probing PATH for a binary.
  • Install with --debug because we don't need a speedy version and otherwise Cargo can take a significant amount of time to optimize.

I believe those points would cover my concerns at least!

@eddyb eddyb force-pushed the vendeur-tres-bien branch 4 times, most recently from ffc1f6a to 1bd57bf Compare February 12, 2017 20:00
@alexcrichton
Copy link
Member

Everything looks great to me, thanks @eddyb!

As soon as travis goes green on the discheck bot (temporarily enabled here) I think this is good to throw at @bors

@eddyb eddyb force-pushed the vendeur-tres-bien branch 4 times, most recently from a143460 to aa44186 Compare February 12, 2017 23:08
@eddyb
Copy link
Member Author

eddyb commented Feb 12, 2017

Successful build is at https://travis-ci.org/rust-lang/rust/builds/200955072, I'm killing this one.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 12, 2017

📌 Commit aa44186 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 13, 2017
…hton

Automate vendoring by invoking cargo-vendor when building src dist tarballs.

This avoids rust-lang#39633 bringing the `src/vendor` checked into git by rust-lang#37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run `cargo vendor` during the `dist src` step is sound.

However, the only way to be sure `cargo-vendor` exists is to run `cargo install --force cargo-vendor`, which will recompile it every time (not passing `--force` means you can't tell between "already exists" and "build error"). ~~This is quite suboptimal and I'd like to somehow do it in each `Dockerfile` that would need it.~~

* [ ] Cache `CARGO_HOME` (i.e. `~/.cargo`) between CI runs
  * `bin/cargo-vendor` and the actual caches are the relevant bits
* [x] Do not build `cargo-vendor` all the time
  * ~~Maybe detect `~/.cargo/bin/cargo-vendor` already exists?~~
  * ~~Could also try to build it in a `Dockerfile` but do we have `cargo`/`rustc` there?~~
  * Final solution: check `cargo install --list` for a line starting with `cargo-vendor `

cc @rust-lang/tools
@bors
Copy link
Contributor

bors commented Feb 13, 2017

⌛ Testing commit aa44186 with merge 311db02...

@bors
Copy link
Contributor

bors commented Feb 13, 2017

💔 Test failed - status-travis

@eddyb
Copy link
Member Author

eddyb commented Feb 13, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

📌 Commit 915d2f0 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

⌛ Testing commit 915d2f0 with merge ba56cf2...

@bors
Copy link
Contributor

bors commented Feb 13, 2017

💔 Test failed - status-travis

@eddyb
Copy link
Member Author

eddyb commented Feb 13, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

📌 Commit e104705 has been approved by alexcrichton

@eddyb
Copy link
Member Author

eddyb commented Feb 13, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

📌 Commit 7bfd156 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

⌛ Testing commit 7bfd156 with merge 702160d...

@bors
Copy link
Contributor

bors commented Feb 13, 2017

💔 Test failed - status-travis

@eddyb
Copy link
Member Author

eddyb commented Feb 13, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

📌 Commit d29f0bc has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 13, 2017

⌛ Testing commit d29f0bc with merge 19dcf06...

@bors
Copy link
Contributor

bors commented Feb 14, 2017

💔 Test failed - status-travis

@steveklabnik
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 14, 2017

⌛ Testing commit d29f0bc with merge c03f12e...

bors added a commit that referenced this pull request Feb 14, 2017
Automate vendoring by invoking cargo-vendor when building src dist tarballs.

This avoids #39633 bringing the `src/vendor` checked into git by #37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run `cargo vendor` during the `dist src` step is sound.

However, the only way to be sure `cargo-vendor` exists is to run `cargo install --force cargo-vendor`, which will recompile it every time (not passing `--force` means you can't tell between "already exists" and "build error"). ~~This is quite suboptimal and I'd like to somehow do it in each `Dockerfile` that would need it.~~

* [ ] Cache `CARGO_HOME` (i.e. `~/.cargo`) between CI runs
  * `bin/cargo-vendor` and the actual caches are the relevant bits
* [x] Do not build `cargo-vendor` all the time
  * ~~Maybe detect `~/.cargo/bin/cargo-vendor` already exists?~~
  * ~~Could also try to build it in a `Dockerfile` but do we have `cargo`/`rustc` there?~~
  * Final solution: check `cargo install --list` for a line starting with `cargo-vendor `

cc @rust-lang/tools
@bors
Copy link
Contributor

bors commented Feb 14, 2017

💔 Test failed - status-appveyor

@eddyb
Copy link
Member Author

eddyb commented Feb 14, 2017

@bors retry

  • more AppVeyor network nonsense

@bors
Copy link
Contributor

bors commented Feb 14, 2017

⌛ Testing commit d29f0bc with merge 48bc082...

bors added a commit that referenced this pull request Feb 14, 2017
Automate vendoring by invoking cargo-vendor when building src dist tarballs.

This avoids #39633 bringing the `src/vendor` checked into git by #37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run `cargo vendor` during the `dist src` step is sound.

However, the only way to be sure `cargo-vendor` exists is to run `cargo install --force cargo-vendor`, which will recompile it every time (not passing `--force` means you can't tell between "already exists" and "build error"). ~~This is quite suboptimal and I'd like to somehow do it in each `Dockerfile` that would need it.~~

* [ ] Cache `CARGO_HOME` (i.e. `~/.cargo`) between CI runs
  * `bin/cargo-vendor` and the actual caches are the relevant bits
* [x] Do not build `cargo-vendor` all the time
  * ~~Maybe detect `~/.cargo/bin/cargo-vendor` already exists?~~
  * ~~Could also try to build it in a `Dockerfile` but do we have `cargo`/`rustc` there?~~
  * Final solution: check `cargo install --list` for a line starting with `cargo-vendor `

cc @rust-lang/tools
@bors
Copy link
Contributor

bors commented Feb 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 48bc082 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants