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

rustbuild: install improvements #42067

Merged
merged 2 commits into from
May 19, 2017
Merged

rustbuild: install improvements #42067

merged 2 commits into from
May 19, 2017

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
Member

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

&mandir, &empty_dir);

t!(fs::remove_dir_all(&empty_dir));
}

fn install_sh(build: &Build, package: &str, name: &str, version: &str, stage: u32, host: &str,
fn install_sh(build: &Build, package: &str, name: &str, version: &str, stage: u32, host: Option<&str>,
Copy link
Member

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

Choose a reason for hiding this comment

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

Sure, will do

@@ -152,6 +152,9 @@
# known-good version of OpenSSL, compile it, and link it to Cargo.
#openssl-static = false

# Disable the vendoring of libs when creating dist archives
# disable-cargo-vendor = false
Copy link
Member

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

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

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

Yeah I'm ok with that too!

…enabled

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
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

@bors: r+

Thanks!

@bors
Copy link
Contributor

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
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 added a commit to Mark-Simulacrum/rust that referenced this pull request May 18, 2017
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.
@bors
Copy link
Contributor

bors commented May 19, 2017

⌛ Testing commit 801e2b7 with merge a3ec3c9...

@bors
Copy link
Contributor

bors commented May 19, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 19, 2017
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.
bors added a commit that referenced this pull request May 19, 2017
@bors bors merged commit 801e2b7 into rust-lang:master May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants