Produce source package in rust-installer format #34366

Merged
merged 2 commits into from Aug 14, 2016

Conversation

Projects
None yet
@Diggsey
Contributor

Diggsey commented Jun 19, 2016

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the rust-src-image directory when it's used by multiple make rules.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jun 19, 2016

Collaborator

r? @brson

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

Collaborator

rust-highfive commented Jun 19, 2016

r? @brson

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 19, 2016

Member

Thanks! Could you also update src/bootstrap/build/dist.rs with a rule for this as well?

Member

alexcrichton commented Jun 19, 2016

Thanks! Could you also update src/bootstrap/build/dist.rs with a rule for this as well?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Jun 19, 2016

Contributor

The rustbuild system doesn't seem to produce a source tarball at all right now - should I make it produce the vanilla tarball in addition to the rust-installer package?

Contributor

Diggsey commented Jun 19, 2016

The rustbuild system doesn't seem to produce a source tarball at all right now - should I make it produce the vanilla tarball in addition to the rust-installer package?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 20, 2016

Member

Oh whoops! Eh it's fine to not do that as part of this PR if you'd prefer, but if you wanna throw it in I'm also fine with that!

Member

alexcrichton commented Jun 20, 2016

Oh whoops! Eh it's fine to not do that as part of this PR if you'd prefer, but if you wanna throw it in I'm also fine with that!

+ $(Q)$(S)src/rust-installer/gen-installer.sh \
+ --product-name=Rust \
+ --rel-manifest-dir=rustlib \
+ --success-message=Awesome-Source. \

This comment has been minimized.

@brson

brson Jun 20, 2016

Contributor

lol

@brson

brson Jun 20, 2016

Contributor

lol

-$(PKG_TAR): $(PKG_FILES)
- @$(call E, making dist dir)
- $(Q)rm -Rf tmp/dist/$(PKG_NAME)
- $(Q)mkdir -p tmp/dist/$(PKG_NAME)

This comment has been minimized.

@brson

brson Jun 20, 2016

Contributor

Unfortunately I think we need to keep producing both formats. The old format is for people building rust, the new one is for people installing it for other purposes.

@brson

brson Jun 20, 2016

Contributor

Unfortunately I think we need to keep producing both formats. The old format is for people building rust, the new one is for people installing it for other purposes.

This comment has been minimized.

@brson

brson Jun 20, 2016

Contributor

Oh, I think I see what you are doing to produce both.

@brson

brson Jun 20, 2016

Contributor

Oh, I think I see what you are doing to produce both.

This comment has been minimized.

@Diggsey

Diggsey Jun 21, 2016

Contributor

Yep it should still produce the old .tar.gz exactly as before

@Diggsey

Diggsey Jun 21, 2016

Contributor

Yep it should still produce the old .tar.gz exactly as before

mk/dist.mk
@@ -87,10 +88,11 @@ $(PKG_TAR): $(PKG_FILES)
--exclude=*/llvm/test/*/*/*.ll \
--exclude=*/llvm/test/*/*/*.td \
--exclude=*/llvm/test/*/*/*.s \
- -c $(UNROOTED_PKG_FILES) | tar -x -f - -C tmp/dist/$(PKG_NAME)
+ -c $(UNROOTED_PKG_FILES) | tar -x -f - -C tmp/dist/$(SRC_PKG_NAME)-image/lib/rust-lib/rust-src

This comment has been minimized.

@brson

brson Jun 20, 2016

Contributor

rust-lib should be rustlib. It's the name of the directory we stuff arbitrary things into.

@brson

brson Jun 20, 2016

Contributor

rust-lib should be rustlib. It's the name of the directory we stuff arbitrary things into.

This comment has been minimized.

@Diggsey

Diggsey Jun 21, 2016

Contributor

Yeah, I've already corrected that locally

@Diggsey

Diggsey Jun 21, 2016

Contributor

Yeah, I've already corrected that locally

mk/dist.mk
@$(call E, making $@)
- $(Q)tar -czf $(PKG_TAR) -C tmp/dist $(PKG_NAME)
- $(Q)rm -Rf tmp/dist/$(PKG_NAME)
+ $(Q)tar -czf $(PKG_TAR) -C tmp/dist/$(SRC_PKG_NAME)-image/lib/rust-lib rust-src --transform 's,^rust-src,$(PKG_NAME),S'

This comment has been minimized.

@brson

brson Jun 20, 2016

Contributor

What does --transform 's,^rust-src,$(PKG_NAME),S' do?

@brson

brson Jun 20, 2016

Contributor

What does --transform 's,^rust-src,$(PKG_NAME),S' do?

This comment has been minimized.

@Diggsey

Diggsey Jun 21, 2016

Contributor

Both source packages are built from the same image, and the directory name is rust-src, but tar uses the directory name as the root directory in the archive, whereas we used to name the root directory $(PKG_NAME), so the --transform option rewrites all the paths so that the resulting .tar.gz matches exactly the format we used before.

@Diggsey

Diggsey Jun 21, 2016

Contributor

Both source packages are built from the same image, and the directory name is rust-src, but tar uses the directory name as the root directory in the archive, whereas we used to name the root directory $(PKG_NAME), so the --transform option rewrites all the paths so that the resulting .tar.gz matches exactly the format we used before.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 20, 2016

Contributor

cc #19535

Contributor

brson commented Jun 20, 2016

cc #19535

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 20, 2016

Contributor

So after installation of the rust-src package the full Rust source will be at lib/rustlib/rust-src meaning the source to std will be at lib/rustlib/rust-src/src/libstd.

We've also discussed about the potential to need other source installations, but without concrete use cases. You might imagine that rust-src gets split into rust-src, rust-core-src, rustc-src etc. or we add a cargo-src. Should we consider making the scheme for storing the source expandable by putting this at e.g. lib/rust/lib/src/rust/?

cc @phildawes @alexreg @MarkSwanson this PR is deciding the scheme for storing the installed Rust source on disk. Once we deploy it it will be very hard to change.

cc brson/multirust#77

Contributor

brson commented Jun 20, 2016

So after installation of the rust-src package the full Rust source will be at lib/rustlib/rust-src meaning the source to std will be at lib/rustlib/rust-src/src/libstd.

We've also discussed about the potential to need other source installations, but without concrete use cases. You might imagine that rust-src gets split into rust-src, rust-core-src, rustc-src etc. or we add a cargo-src. Should we consider making the scheme for storing the source expandable by putting this at e.g. lib/rust/lib/src/rust/?

cc @phildawes @alexreg @MarkSwanson this PR is deciding the scheme for storing the installed Rust source on disk. Once we deploy it it will be very hard to change.

cc brson/multirust#77

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 20, 2016

Contributor

It might also be worth considering the impact on side-by-side installation. Right now it's in theory vaguely possible to install multiple Rust's right on top of each other because of the disambiguated crate names, though in practice there are artifacts in rustlib that are not disambiguated. Putting a single copy of the source at a known location in rustlib means that only one can exist at a time. This is fine for rustup, but not fine for e.g. Linux distros.

I think I'm kinda fine just saying that every rustlib belongs to a single Rust installation, but it does put a nail in the coffin of any hope of treating rustlib as a more general 'global assembly cache'.

Contributor

brson commented Jun 20, 2016

It might also be worth considering the impact on side-by-side installation. Right now it's in theory vaguely possible to install multiple Rust's right on top of each other because of the disambiguated crate names, though in practice there are artifacts in rustlib that are not disambiguated. Putting a single copy of the source at a known location in rustlib means that only one can exist at a time. This is fine for rustup, but not fine for e.g. Linux distros.

I think I'm kinda fine just saying that every rustlib belongs to a single Rust installation, but it does put a nail in the coffin of any hope of treating rustlib as a more general 'global assembly cache'.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 21, 2016

Contributor

cc @jauhien @vadimcn @sylvestre packaging people. This PR is trying to finalize the storage location for the Rust source code (#19535), for use by various tools, and I want to check what impact it might have on packagers. In rustup it'll be an optional package called rust-src, and tentatively installed to lib/rustlib/rust-src.

The concern I have regarding distro packaging is the impact on side-by-side installation. In particular, if SxS is achieved by just installing multiple revisions of Rust directly on top of each other then this src location doesn't work because multiple revisions will want to put the source there.

From previous conversations with @jauhien my impression was that at least Gentoo had a more sophisticated SxS installation scheme of their own. What I would like to do for simplicity is abandon any pretense that rustlib is shared by multiple versions of Rust. Let distros deal with SxS installation their own way and reexamine how it impacts us when the time comes.

Of course distros may not want to use our scheme for installing the source at all, but then they are going to have to deal with the integration with tools separately.

Contributor

brson commented Jun 21, 2016

cc @jauhien @vadimcn @sylvestre packaging people. This PR is trying to finalize the storage location for the Rust source code (#19535), for use by various tools, and I want to check what impact it might have on packagers. In rustup it'll be an optional package called rust-src, and tentatively installed to lib/rustlib/rust-src.

The concern I have regarding distro packaging is the impact on side-by-side installation. In particular, if SxS is achieved by just installing multiple revisions of Rust directly on top of each other then this src location doesn't work because multiple revisions will want to put the source there.

From previous conversations with @jauhien my impression was that at least Gentoo had a more sophisticated SxS installation scheme of their own. What I would like to do for simplicity is abandon any pretense that rustlib is shared by multiple versions of Rust. Let distros deal with SxS installation their own way and reexamine how it impacts us when the time comes.

Of course distros may not want to use our scheme for installing the source at all, but then they are going to have to deal with the integration with tools separately.

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Jun 21, 2016

Contributor

I've updated rustbuild to produce the two source distributions, and fixed the typo in rustlib.

Contributor

Diggsey commented Jun 21, 2016

I've updated rustbuild to produce the two source distributions, and fixed the typo in rustlib.

src/bootstrap/build/dist.rs
@@ -287,6 +289,97 @@ pub fn std(build: &Build, compiler: &Compiler, target: &str) {
t!(fs::remove_dir_all(&image));
}
+/// Creates the `rust-std` installer component as compiled by `compiler` for the
+/// target `target`.

This comment has been minimized.

@durka

durka Jun 21, 2016

Contributor

copy-pasted comment?

@durka

durka Jun 21, 2016

Contributor

copy-pasted comment?

This comment has been minimized.

@Diggsey

Diggsey Jun 21, 2016

Contributor

Oops

@Diggsey

Diggsey Jun 21, 2016

Contributor

Oops

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 21, 2016

Member

I'd personally lean a bit towards rustlib/src/rust instead of rustlib/rust-src just to allow for Cargo one day (and perhaps other tools like rustfmt), but I don't think we'll go too far in the directon of rustlib/rust-core-src as it'd basically just be a space optimization.

Member

alexcrichton commented Jun 21, 2016

I'd personally lean a bit towards rustlib/src/rust instead of rustlib/rust-src just to allow for Cargo one day (and perhaps other tools like rustfmt), but I don't think we'll go too far in the directon of rustlib/rust-core-src as it'd basically just be a space optimization.

@MarkSwanson

This comment has been minimized.

Show comment
Hide comment
@MarkSwanson

MarkSwanson Jun 21, 2016

I'm glad this is being worked on!
I like rustlib/src/rust.
I like making it easy to perhaps add the sources for cargo / rustfmt etc. Maybe llvm headers. clippy? etc.

I'm glad this is being worked on!
I like rustlib/src/rust.
I like making it easy to perhaps add the sources for cargo / rustfmt etc. Maybe llvm headers. clippy? etc.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 23, 2016

Contributor

Let's go with rustlib/src/rust and keep pressing forward.

Contributor

brson commented Jun 23, 2016

Let's go with rustlib/src/rust and keep pressing forward.

@sylvestre

This comment has been minimized.

Show comment
Hide comment
@sylvestre

sylvestre Jun 24, 2016

Contributor

Sorry for the dumb question but what is SxS ?
Besides that, we don't ship the sources as a package in Debian. We are using the tag from github to get the tarball.

Contributor

sylvestre commented Jun 24, 2016

Sorry for the dumb question but what is SxS ?
Besides that, we don't ship the sources as a package in Debian. We are using the tag from github to get the tarball.

@sorear

This comment has been minimized.

Show comment
Hide comment
@sorear

sorear Jun 25, 2016

Contributor

@sylvestre side-by-side installation, in other words having more than one version of rust (e.g. nightly and stable) installed and usable on the same machine at the same time

Contributor

sorear commented Jun 25, 2016

@sylvestre side-by-side installation, in other words having more than one version of rust (e.g. nightly and stable) installed and usable on the same machine at the same time

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 2, 2016

Contributor

☔️ The latest upstream changes (presumably #34611) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Jul 2, 2016

☔️ The latest upstream changes (presumably #34611) made this pull request unmergeable. Please resolve the merge conflicts.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 8, 2016

Contributor

Something to consider re the std-aware cargo RFC: in order to teach cargo how to build std, we're most likely going to end up having cargo look in the sysroot for the source - that is, the source we're packaging in this PR.

So far we've not made any particular commitments about what the 'shape' of that source will like, we're just dumping the source tree into rustlib/src/rust. ISTM though that Cargo might want a deterministic scheme for finding crates with a specific name, so that if it learns that e.g. collections is part of the crate tag, it knows to look in a specific directory for a Cargo.toml corresponding to the collections package.

ISTM there's also some overlap here with the idea of 'local crates.io mirrors' as well. @alexcrichton does it make sense for Cargo to treat part of the sysroot as a standard local crates.io mirror?

I guess I see a few options here:

  • we can go forward with this 'dump' of the raw source into the sysroot and expect tools to just kinda do there best (I imagine they would have a two stage lookup: first, use hardcoded knowledge of the official Rust source tree to find a crate; second walk the tree looking for Cargo.toml files)
  • we can instead plan a more structured layout for source in the sysroot where tools know exactly how to find the source to a specific crate
  • we can go ahead with the current patch but plan on adding the additional structure later.
  • a combination of the above, but structuring the source in the sysroot to fit Cargo's hypothetical 'local registry' layout.

For the sake of expediency I think I'm inclined to just keep going forward with this patch and force tools like Cargo to do their bust to locate the crates in the source tree.

cc @Ericson2314

Contributor

brson commented Jul 8, 2016

Something to consider re the std-aware cargo RFC: in order to teach cargo how to build std, we're most likely going to end up having cargo look in the sysroot for the source - that is, the source we're packaging in this PR.

So far we've not made any particular commitments about what the 'shape' of that source will like, we're just dumping the source tree into rustlib/src/rust. ISTM though that Cargo might want a deterministic scheme for finding crates with a specific name, so that if it learns that e.g. collections is part of the crate tag, it knows to look in a specific directory for a Cargo.toml corresponding to the collections package.

ISTM there's also some overlap here with the idea of 'local crates.io mirrors' as well. @alexcrichton does it make sense for Cargo to treat part of the sysroot as a standard local crates.io mirror?

I guess I see a few options here:

  • we can go forward with this 'dump' of the raw source into the sysroot and expect tools to just kinda do there best (I imagine they would have a two stage lookup: first, use hardcoded knowledge of the official Rust source tree to find a crate; second walk the tree looking for Cargo.toml files)
  • we can instead plan a more structured layout for source in the sysroot where tools know exactly how to find the source to a specific crate
  • we can go ahead with the current patch but plan on adding the additional structure later.
  • a combination of the above, but structuring the source in the sysroot to fit Cargo's hypothetical 'local registry' layout.

For the sake of expediency I think I'm inclined to just keep going forward with this patch and force tools like Cargo to do their bust to locate the crates in the source tree.

cc @Ericson2314

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 8, 2016

Contributor

@Diggsey what are the prospects for whipping this patch back into shape?

Contributor

brson commented Jul 8, 2016

@Diggsey what are the prospects for whipping this patch back into shape?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Jul 8, 2016

Contributor

Just need to fix the merge conflicts then in that case.

Contributor

Diggsey commented Jul 8, 2016

Just need to fix the merge conflicts then in that case.

@eternaleye

This comment has been minimized.

Show comment
Hide comment
@eternaleye

eternaleye Jul 8, 2016

Contributor

@brson: Personally, I'd prefer option 3 - roll this out as is, but plan on adding structure.

In the end, that results in the same two-phase lookup being written into tools, but rather than privileging rustc (and making every other compiler implementation take a perf hit unless they mimic a historical accident, and one that may change in the future if rustc's repo changes anything), it makes the fallback less and less frequently-hit over time.

It's just as expedient for Rust (no extra work before rollout), more expedient for Cargo (implement only the "look for Cargo.toml" path now), and doesn't privilege rustc over future implementations (at least one is already being written as mrustc.)

Contributor

eternaleye commented Jul 8, 2016

@brson: Personally, I'd prefer option 3 - roll this out as is, but plan on adding structure.

In the end, that results in the same two-phase lookup being written into tools, but rather than privileging rustc (and making every other compiler implementation take a perf hit unless they mimic a historical accident, and one that may change in the future if rustc's repo changes anything), it makes the fallback less and less frequently-hit over time.

It's just as expedient for Rust (no extra work before rollout), more expedient for Cargo (implement only the "look for Cargo.toml" path now), and doesn't privilege rustc over future implementations (at least one is already being written as mrustc.)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 9, 2016

Member

@brson this is basically already in the shape of a "path source" that Cargo expects today (or a git repository), so it should interoperate regardless. If we want to work with a local registry or vendored directory then we'd probably want more structure, but as you say I'm fine adding that later.

Member

alexcrichton commented Jul 9, 2016

@brson this is basically already in the shape of a "path source" that Cargo expects today (or a git repository), so it should interoperate regardless. If we want to work with a local registry or vendored directory then we'd probably want more structure, but as you say I'm fine adding that later.

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Jul 9, 2016

Contributor

Fixed conflicts.

Contributor

Diggsey commented Jul 9, 2016

Fixed conflicts.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jul 10, 2016

Contributor

@alexcrichton I think we'll need a real registery, so std = 1.9 Just Works™. I'm thinking a way to export a workspace into a registery, or treat a workspace as a registery might be a good idea in general. [Even when working in a workspace, maybe members should overrides crates.io.]

Contributor

Ericson2314 commented Jul 10, 2016

@alexcrichton I think we'll need a real registery, so std = 1.9 Just Works™. I'm thinking a way to export a workspace into a registery, or treat a workspace as a registery might be a good idea in general. [Even when working in a workspace, maybe members should overrides crates.io.]

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 11, 2016

Contributor

@bors r+

Contributor

brson commented Jul 11, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 11, 2016

Contributor

📌 Commit 21ccad9 has been approved by brson

Contributor

bors commented Jul 11, 2016

📌 Commit 21ccad9 has been approved by brson

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jul 12, 2016

Contributor

In rust-lang/cargo#2361 (comment) I propose that workspace members act like a registry, in that version-based dependencies can resolve to members of the workspace (with higher priority than crates.io). This would in turn allow rustbuild to avoid the use of path dependencies entirely, which would also make converting to a real registry easier.

Alternatively, Cargo could use workspaces (other than the current one) as an alternative registry format, which would avoid the need to do any conversion at all.

Contributor

Ericson2314 commented Jul 12, 2016

In rust-lang/cargo#2361 (comment) I propose that workspace members act like a registry, in that version-based dependencies can resolve to members of the workspace (with higher priority than crates.io). This would in turn allow rustbuild to avoid the use of path dependencies entirely, which would also make converting to a real registry easier.

Alternatively, Cargo could use workspaces (other than the current one) as an alternative registry format, which would avoid the need to do any conversion at all.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 12, 2016

Contributor

⌛️ Testing commit 21ccad9 with merge 91ee936...

Contributor

bors commented Jul 12, 2016

⌛️ Testing commit 21ccad9 with merge 91ee936...

bors added a commit that referenced this pull request Jul 12, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 12, 2016

Contributor

💔 Test failed - auto-mac-64-opt-rustbuild

Contributor

bors commented Jul 12, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 14, 2016

Contributor

@Diggsey It's a legit failure. And I see a few problems with this patch now.

First, as far as I can tell, the packaging code should not even be running in this configuration. @alexcrichton can you see why rustbuild is trying to build the source package here?

Furthermore, tar on mac doesn't support --exclude-vcs. The way we avoided this in the makefiles is by just not producing the source tarball there, but with this patch tar is being used everywhere.

But that leads to another issue, that the package being produced here is just rust-src, not distinguished by a triple. This means every host is going to be building this artifact. I'm not what will happen when rust-buildbot sees that on a dist build.

Contributor

brson commented Jul 14, 2016

@Diggsey It's a legit failure. And I see a few problems with this patch now.

First, as far as I can tell, the packaging code should not even be running in this configuration. @alexcrichton can you see why rustbuild is trying to build the source package here?

Furthermore, tar on mac doesn't support --exclude-vcs. The way we avoided this in the makefiles is by just not producing the source tarball there, but with this patch tar is being used everywhere.

But that leads to another issue, that the package being produced here is just rust-src, not distinguished by a triple. This means every host is going to be building this artifact. I'm not what will happen when rust-buildbot sees that on a dist build.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 14, 2016

Member

@brson as part of make check rustbuild runs the equivalent of make dist because it breaks so often

Member

alexcrichton commented Jul 14, 2016

@brson as part of make check rustbuild runs the equivalent of make dist because it breaks so often

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 14, 2016

Contributor

I'd say the next step here is to get rid of --exclude-vcs. In rustbuild we can just do the copy recursive filecopy ourselves. In the makefiles I might suggest just eliminating that part of the build on not-windows like the existing source build.

Contributor

brson commented Jul 14, 2016

I'd say the next step here is to get rid of --exclude-vcs. In rustbuild we can just do the copy recursive filecopy ourselves. In the makefiles I might suggest just eliminating that part of the build on not-windows like the existing source build.

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 7, 2016

Contributor

Finally got around to re-implementing the --exclude-vcs, etc. functionality.

Contributor

Diggsey commented Aug 7, 2016

Finally got around to re-implementing the --exclude-vcs, etc. functionality.

src/build_helper/lib.rs
@@ -70,6 +70,13 @@ pub fn output(cmd: &mut Command) -> String {
String::from_utf8(output.stdout).unwrap()
}
+pub fn spawn(cmd: &mut Command) -> Child {

This comment has been minimized.

@brson

brson Aug 9, 2016

Contributor

It looks to me like this function is unused. Can you remove it?

@brson

brson Aug 9, 2016

Contributor

It looks to me like this function is unused. Can you remove it?

This comment has been minimized.

@Diggsey

Diggsey Aug 9, 2016

Contributor

Removed!

@Diggsey

Diggsey Aug 9, 2016

Contributor

Removed!

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 9, 2016

Contributor

@bors r+

Contributor

brson commented Aug 9, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 9, 2016

Contributor

📌 Commit 777bd6f has been approved by brson

Contributor

bors commented Aug 9, 2016

📌 Commit 777bd6f has been approved by brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 9, 2016

Contributor

⌛️ Testing commit 777bd6f with merge a63a01f...

Contributor

bors commented Aug 9, 2016

⌛️ Testing commit 777bd6f with merge a63a01f...

bors added a commit that referenced this pull request Aug 9, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 9, 2016

Contributor

💔 Test failed - auto-win-msvc-64-cargotest

Contributor

bors commented Aug 9, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 9, 2016

Contributor

Failure looks to be spurious, and was caused by remove_dir_all being broken on windows (#29497)

Contributor

Diggsey commented Aug 9, 2016

Failure looks to be spurious, and was caused by remove_dir_all being broken on windows (#29497)

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 10, 2016

Contributor

@bors retry

Contributor

brson commented Aug 10, 2016

@bors retry

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 10, 2016

Contributor

⌛️ Testing commit 777bd6f with merge 0eefd73...

Contributor

bors commented Aug 10, 2016

⌛️ Testing commit 777bd6f with merge 0eefd73...

bors added a commit that referenced this pull request Aug 10, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 10, 2016

Contributor

💔 Test failed - auto-mac-64-opt-rustbuild

Contributor

bors commented Aug 10, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 11, 2016

Contributor

Ugh... That should fix it for mac. 🍎 🔫

Contributor

Diggsey commented Aug 11, 2016

Ugh... That should fix it for mac. 🍎 🔫

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 11, 2016

Contributor

@bors r+

Contributor

brson commented Aug 11, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 11, 2016

Contributor

📌 Commit 511c0d1 has been approved by brson

Contributor

bors commented Aug 11, 2016

📌 Commit 511c0d1 has been approved by brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 11, 2016

Contributor

⌛️ Testing commit 511c0d1 with merge 7f6705d...

Contributor

bors commented Aug 11, 2016

⌛️ Testing commit 511c0d1 with merge 7f6705d...

bors added a commit that referenced this pull request Aug 11, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 11, 2016

Contributor

💔 Test failed - auto-mac-64-opt-rustbuild

Contributor

bors commented Aug 11, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Aug 11, 2016

@bors: retry

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 12, 2016

Contributor

⌛️ Testing commit 511c0d1 with merge 1447527...

Contributor

bors commented Aug 12, 2016

⌛️ Testing commit 511c0d1 with merge 1447527...

bors added a commit that referenced this pull request Aug 12, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 12, 2016

Contributor

💔 Test failed - auto-mac-64-opt-rustbuild

Contributor

bors commented Aug 12, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

Produce source package in rust-installer format in addition to vanill…
…a tarball

Copy source files from rust code

Add missing wildcard

Remove unused function

Remove use of tar --transform
@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 12, 2016

Contributor

I think this is caused by the addition of the metadata section and package checksums to the Cargo.lock format in new versions of cargo, but I have no idea why other PRs aren't affected, or why a different version of cargo would be being used.

Contributor

Diggsey commented Aug 12, 2016

I think this is caused by the addition of the metadata section and package checksums to the Cargo.lock format in new versions of cargo, but I have no idea why other PRs aren't affected, or why a different version of cargo would be being used.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Aug 12, 2016

Member

Other PRs wouldn't be affected if they didn't change Cargo.toml as far as I know; I think this is true of most PRs.

Member

Mark-Simulacrum commented Aug 12, 2016

Other PRs wouldn't be affected if they didn't change Cargo.toml as far as I know; I think this is true of most PRs.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 12, 2016

Member

Lots of Cargo.lock exists that shouldn't in those logs, for example libpanic_unwind/Cargo.lock shouldn't ever exist. I wonder how they're being created?

The Cargo in use to generate these is pinned, it's downloaded as part of the bootstrap process

Member

alexcrichton commented Aug 12, 2016

Lots of Cargo.lock exists that shouldn't in those logs, for example libpanic_unwind/Cargo.lock shouldn't ever exist. I wonder how they're being created?

The Cargo in use to generate these is pinned, it's downloaded as part of the bootstrap process

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 13, 2016

Contributor

@alexcrichton Those files exist in the repository...

Contributor

Diggsey commented Aug 13, 2016

@alexcrichton Those files exist in the repository...

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 13, 2016

Contributor

This should fix the tidy check.

There were two problems:

  1. This specific check did not work on windows. msys's git diff-index ignores filenames with backslashes in, so the files would always show as unchanged. I fixed this by replacing backslashes with forward slashes before passing it to git.
  2. git diff-index produces false positives after running make dist. I still haven't figured out why this happens, but using git diff instead fixes the problem.
Contributor

Diggsey commented Aug 13, 2016

This should fix the tidy check.

There were two problems:

  1. This specific check did not work on windows. msys's git diff-index ignores filenames with backslashes in, so the files would always show as unchanged. I fixed this by replacing backslashes with forward slashes before passing it to git.
  2. git diff-index produces false positives after running make dist. I still haven't figured out why this happens, but using git diff instead fixes the problem.
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Aug 14, 2016

@bors: r=brson b3908d0

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 14, 2016

Contributor

⌛️ Testing commit b3908d0 with merge 92ae4ce...

Contributor

bors commented Aug 14, 2016

⌛️ Testing commit b3908d0 with merge 92ae4ce...

bors added a commit that referenced this pull request Aug 14, 2016

Auto merge of #34366 - Diggsey:rust-src-pkg, r=brson
Produce source package in rust-installer format

See rust-lang-deprecated/rust-buildbot#102

There may be a better way to do this, wasn't sure how to clean-up the `rust-src-image` directory when it's used by multiple make rules.

@bors bors merged commit b3908d0 into rust-lang:master Aug 14, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Aug 14, 2016

Contributor

🎆

Contributor

Diggsey commented Aug 14, 2016

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment