Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upInclude source packages in manifest #102
Conversation
rust-highfive
assigned
brson
May 21, 2016
This comment has been minimized.
This comment has been minimized.
rust-highfive
commented
May 21, 2016
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
|
The source packages are a little different from other packages, in that they are not in the usual "installer" format (they don't have a components file describing how to install the package). From rustup's POV, I plan on treating packages which don't contain a components file as "pure data" packages, which will just get copied to a suitably named subdirectory within the toolchain directory. |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the PR! I suspect that @brson has many thoughts on this, so I'm somewhat hesitant to deploy a new format in the interim before he weighs in. I personally feel somewhat odd about using "*" as the "target" here, but so long as it works it's just an implementation detail so I'm not too worried. |
This comment has been minimized.
This comment has been minimized.
|
Something like this is important I think to support the IDE integration tools which need copies of libcore and libstd for indexing. Having the installer put the source files in $prefix/src/ would make the sources easier to find. |
This comment has been minimized.
This comment has been minimized.
|
Ah yeah we definitely want to include them! This is just a question of how we modify our metadata to include info about where to download them from. |
thommay
referenced this pull request
May 31, 2016
Closed
Fetch rust sources when install toolchain. (for racer) #505
Diggsey
referenced this pull request
May 31, 2016
Closed
Optionally download source code along with toolchain #37
This comment has been minimized.
This comment has been minimized.
|
Thanks for moving on this. It looks to me like this is including the existing source tarball in the manifest, which is not in the rust-installer format. If we were to go that route then clients interpreting the manifest would have to know that tarball is special, and I prefer not to do that. So before we do this we need to modify the rust build system to produce a new source artifact that is built by rust-installer, so that rustup can treat it like any other installer. The current source tarball is called "rustc-$version-src.tar.gz", and we'll want the build system to additionally produce a rust-installer tarball called "rustc-src-$version.tar.gz". As to the installation location, it should probably be The question of what to do with the target name is tricky. Fortunately rust-installer itself doesn't know anything about targets - it's just part of the package name. rustup though does care about targets and expects every component to have a name/target pair, so it will need to be extended to deal with components that don't have a target. I think using an asterisks here will be fine but I'm not sure of all the details it will affect. |
This comment has been minimized.
This comment has been minimized.
|
I'd prefer to use This shouldn't cause much of a problem for rustup: the modifications (if any are even needed) should be fairly self contained as it only uses the package targets to immediately lookup that package within the manifest. I chose to use asterisks because it required the minimal number of changes, so made sense to do that until I could get some feedback, but modifying the manifest format also works. |
This comment has been minimized.
This comment has been minimized.
|
Oh, yes I prefer |
matklad
referenced this pull request
Jun 9, 2016
Closed
Phase 1 of stdlib dependencies (RFC #1133) #2768
Diggsey
referenced this pull request
Jun 19, 2016
Merged
Produce source package in rust-installer format #34366
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
liuchong
commented
Jul 21, 2016
|
will it also set a "RUST_SRC_PATH" env var? racer and other apps are using this. |
This comment has been minimized.
This comment has been minimized.
retep998
commented
Jul 21, 2016
|
@liuchong That would probably be handled by a combination of rustup.rs and whatever editor tooling you're using. This PR is just allowing for source packages to be created on the buildbots. |
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Aug 9, 2016
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Aug 10, 2016
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Aug 11, 2016
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Aug 12, 2016
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Aug 14, 2016
brson
reviewed
Aug 16, 2016
| # The build system treats source packages as a separate target for `rustc` | ||
| # but for rustup we'd like to treat them as a completely separate package. | ||
| url1 = s3_addy + "/" + dist_dir + "/" + date + "/rustc-" + version + "-src.tar.gz" | ||
| url2 = s3_addy + "/" + dist_dir + "/" + date + "/rustc-" + maybe_channel + "-src.tar.gz" |
This comment has been minimized.
This comment has been minimized.
brson
Aug 16, 2016
Contributor
@Diggsey It looks to me like the only thing that needs to change is the naming scheme here. It's rust-src-$version-or-channel.tar.gz now.
This comment has been minimized.
This comment has been minimized.
|
Fixed up the name |
This comment has been minimized.
This comment has been minimized.
|
I'm going to test this in the dev environment to make sure existing rustup deployments don't break on the new metadata. I suspect there are some tweaks needed still. Starting a build in dev now. |
This comment has been minimized.
This comment has been minimized.
|
The only thing that needs to change is that |
Diggsey commentedMay 21, 2016
This will allow rustup to install the rust source as an optional extension for a given toolchain.