Skip to content

Only use cargo-vendor if building from git sources #41047

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Apr 7, 2017

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 3, 2017

The only time we need to vendor sources is when building from git. If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common Build::src_is_git flag, and then uses it in the dist-src
target to decide whether to install or use cargo-vendor at all.

Fixes #41042.

@rust-highfive
Copy link
Contributor

r? @brson

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

@cuviper
Copy link
Member Author

cuviper commented Apr 3, 2017

r? @alexcrichton

I'd also nominate this for beta as it's a slight regression from 1.16, which didn't try to access the network.

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Apr 3, 2017
@@ -233,6 +234,7 @@ impl Build {
};
let rust_info = channel::GitInfo::new(&src);
let cargo_info = channel::GitInfo::new(&src.join("cargo"));
let src_is_git = src.join(".git").is_dir();
Copy link
Member

Choose a reason for hiding this comment

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

This won't work if rust itself is a git submodule. Checking if .git exists at all would be an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I thought it was better to be more precise, but I see your point. I'll check just for existence.

Copy link
Member

@aidanhs aidanhs Apr 3, 2017

Choose a reason for hiding this comment

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

(i.e. using fs::metadata as before) edit: oops, didn't see your comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Or Path::exists() is the same metadata check, but shorter. :)

@alexcrichton
Copy link
Member

@bors: r+

Thanks @cuviper!

@bors
Copy link
Collaborator

bors commented Apr 3, 2017

📌 Commit 4d32ff4 has been approved by alexcrichton

let mut has_cargo_vendor = false;
let mut cmd = Command::new(&build.cargo);
for line in output(cmd.arg("install").arg("--list")).lines() {
has_cargo_vendor |= line.starts_with("cargo-vendor ");
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this allow the build to use an installed cargo-vendor of the wrong version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe so, but FWIW I didn't really change any of these lines, only their indentation.

@arielb1
Copy link
Contributor

arielb1 commented Apr 4, 2017

beta-nominate this?

@alexcrichton alexcrichton added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 4, 2017
@cuviper
Copy link
Member Author

cuviper commented Apr 4, 2017

Should I open a beta backport PR? It seems like you're rolling these in batches lately.
(It's an easy backport -- the diff applies cleanly as-is.)

@alexcrichton
Copy link
Member

@cuviper yeah if you're willing please feel free! We'll batch up if necessary but it's not required.

@cuviper
Copy link
Member Author

cuviper commented Apr 4, 2017

#41069 is for beta.

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 4, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 6, 2017
Only use cargo-vendor if building from git sources

The only time we need to vendor sources is when building from git.  If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common `Build::src_is_git` flag, and then uses it in the dist-src
target to decide whether to install or use `cargo-vendor` at all.

Fixes rust-lang#41042.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 6, 2017
Only use cargo-vendor if building from git sources

The only time we need to vendor sources is when building from git.  If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common `Build::src_is_git` flag, and then uses it in the dist-src
target to decide whether to install or use `cargo-vendor` at all.

Fixes rust-lang#41042.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 6, 2017
Only use cargo-vendor if building from git sources

The only time we need to vendor sources is when building from git.  If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common `Build::src_is_git` flag, and then uses it in the dist-src
target to decide whether to install or use `cargo-vendor` at all.

Fixes rust-lang#41042.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 7, 2017
Only use cargo-vendor if building from git sources

The only time we need to vendor sources is when building from git.  If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common `Build::src_is_git` flag, and then uses it in the dist-src
target to decide whether to install or use `cargo-vendor` at all.

Fixes rust-lang#41042.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 7, 2017
Only use cargo-vendor if building from git sources

The only time we need to vendor sources is when building from git.  If one is
building from a rustc source tarball, everything should already be in place.
This also matters for distros which do offline builds, as they can't install
cargo-vendor this way.

This adds a common `Build::src_is_git` flag, and then uses it in the dist-src
target to decide whether to install or use `cargo-vendor` at all.

Fixes rust-lang#41042.
bors added a commit that referenced this pull request Apr 7, 2017
Rollup of 9 pull requests

- Successful merges: #40797, #41047, #41056, #41061, #41075, #41080, #41120, #41130, #41131
- Failed merges:
@bors
Copy link
Collaborator

bors commented Apr 7, 2017

⌛ Testing commit 4d32ff4 with merge 53f4bc3...

@bors bors merged commit 4d32ff4 into rust-lang:master Apr 7, 2017
@cuviper cuviper deleted the src_is_git branch September 26, 2017 06:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants