Skip to content

rustbuild: install improvements#42067

Merged
bors merged 2 commits intorust-lang:masterfrom
Keruspe:master
May 19, 2017
Merged

rustbuild: install improvements#42067
bors merged 2 commits intorust-lang:masterfrom
Keruspe:master

Conversation

@Keruspe
Copy link
Contributor

@Keruspe Keruspe commented May 17, 2017

Install rust-analysis and rust-src to get in par with what we can get from rustup.
Allow bypassing the vendoring of dependencies. When we only build to install and not to redistribute dist tarballs, that part is not necessary, so avoid trying to install cargo-vendor.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2017
@aidanhs
Copy link
Contributor

aidanhs commented May 18, 2017

Hi @Keruspe, thanks for the PR! Looks like this failed CI because you have some long lines:

[00:01:57] tidy check (x86_64-unknown-linux-gnu)
[00:01:58] tidy error: /checkout/src/bootstrap/install.rs:90: line longer than 100 chars
[00:01:59] some tidy checks failed
[00:01:59] 
[00:01:59] 
[00:01:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/tidy" "/checkout/src" "--no-vendor"
[00:01:59] expected success, got: exit code: 1

alexcrichton
alexcrichton reviewed May 18, 2017
View reviewed changes
src/bootstrap/install.rs Outdated
Copy link
Member

@alexcrichton alexcrichton May 18, 2017

Choose a reason for hiding this comment

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

Mind doing a bit of refactoring while you're here? I feel like a function which takes 13 arguments is... odd

Copy link
Contributor Author

@Keruspe Keruspe May 18, 2017

Choose a reason for hiding this comment

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

Sure, will do

alexcrichton
alexcrichton reviewed May 18, 2017
View reviewed changes
src/bootstrap/config.toml.example Outdated
Copy link
Member

@alexcrichton alexcrichton May 18, 2017

Choose a reason for hiding this comment

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

I think this may not be the intent we wish to convey, perhaps "disable building the source tarball" ?

Copy link
Contributor Author

@Keruspe Keruspe May 18, 2017

Choose a reason for hiding this comment

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

Was not sure how to call that.
Will update the comment, should I change the configuration key name too?

@Keruspe
Copy link
Contributor Author

Keruspe commented May 18, 2017

@alexcrichton Is a refactor of this kind what you had in mind?

@aidanhs that error should be fixed now

@alexcrichton
Copy link
Member

alexcrichton commented May 18, 2017

Looks great @Keruspe, thanks!

For the disable-cargo-vendor what I was thinking is:

  • Disabling cargo vendor produces an "invalid" source tarball, so ideally this wouldn't happen
  • Instead I think for make install and the like you just don't need source tarballs at all
  • Perhaps the install step could just not depend on source tarballs?

@Keruspe
Copy link
Contributor Author

Keruspe commented May 18, 2017

I'll try something like that.
Maybe I should drop that commit for now to get the rest in first, and open another pull request when that part is ready?

@alexcrichton
Copy link
Member

alexcrichton commented May 18, 2017

Yeah I'm ok with that too!

Keruspe added 2 commits May 18, 2017 18:58
@Keruspe
rustbuild: install rust-analysis and rust-src when extended build is …
ecfdc9a 
…enabled

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
rustbuild: refactor install
801e2b7 
Introduce a new Installer object that hold a reference to all the
configured paths for installation

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
Copy link
Contributor Author

Keruspe commented May 18, 2017

Dropped the commit and rebased on top of master

@alexcrichton
Copy link
Member

alexcrichton commented May 18, 2017

@bors: r+

Thanks!

@bors
Copy link
Collaborator

bors commented May 18, 2017

📌 Commit 801e2b7 has been approved by alexcrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 18, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#42067 - Keruspe:master, r=alexcrichton
b384931 
rustbuild: install improvements

Install rust-analysis and rust-src to get in par with what we can get from rustup.
Allow bypassing the vendoring of dependencies. When we only build to install and not to redistribute dist tarballs, that part is not necessary, so avoid trying to install cargo-vendor.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 18, 2017
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 18, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#42067 - Keruspe:master, r=alexcrichton
e4998a7 
rustbuild: install improvements

Install rust-analysis and rust-src to get in par with what we can get from rustup.
Allow bypassing the vendoring of dependencies. When we only build to install and not to redistribute dist tarballs, that part is not necessary, so avoid trying to install cargo-vendor.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 18, 2017
Closed
@bors
Copy link
Collaborator

bors commented May 19, 2017

⌛ Testing commit 801e2b7 with merge a3ec3c9...

@bors
Copy link
Collaborator

bors commented May 19, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented May 19, 2017

@bors: retry

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 19, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#42067 - Keruspe:master, r=alexcrichton
abd1a00 
rustbuild: install improvements

Install rust-analysis and rust-src to get in par with what we can get from rustup.
Allow bypassing the vendoring of dependencies. When we only build to install and not to redistribute dist tarballs, that part is not necessary, so avoid trying to install cargo-vendor.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 19, 2017
Merged
bors added a commit that referenced this pull request May 19, 2017
@bors
Auto merge of #42105 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
5dfcd85 
Rollup of 17 pull requests

- Successful merges: #41870, #41910, #41958, #41971, #42006, #42024, #42037, #42056, #42067, #42070, #42079, #42080, #42082, #42089, #42092, #42096, #42100
- Failed merges:
@bors bors merged commit 801e2b7 into rust-lang:master May 19, 2017
@Keruspe Keruspe mentioned this pull request May 22, 2017
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Keruspe @aidanhs @alexcrichton @bors