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

Generate XZ-compressed tarballs #41600

Merged
merged 5 commits into from
May 4, 2017
Merged

Generate XZ-compressed tarballs #41600

merged 5 commits into from
May 4, 2017

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Apr 28, 2017

Integrate the new rust-installer and extend manifests with keys for xz-compressed tarballs.

One of the steps required for #21724

instead of combining the component and target into a URL.
The same unavailable target value is used in two different places.
Abstracting it makes it easier to update it and recognise its purpose.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@ranma42
Copy link
Contributor Author

ranma42 commented Apr 28, 2017

I tried to add the required packages to the CI infrastructure, but I am not very confident that everything is in place. I trust the CI itself to warn about such issues.
Should I ALLOW_PR=1 to do that?

@ishitatsuyuki
Copy link
Contributor

You can use ALLOW_PR to get rapid feedback; doing it on one of the builders (preferably x86_64-linux-gnu) doesn't jam the queue so much. Make sure you add some sort of WIP tag.

@ranma42 ranma42 changed the title Generate XZ-compressed tarballs [WIP] Generate XZ-compressed tarballs Apr 28, 2017
@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 28, 2017
@nikomatsakis
Copy link
Contributor

Who should review this? Not, I think, me.

@nikomatsakis
Copy link
Contributor

r? @alexcrichton -- feel free to pick someone else, I figured you'd know the right person tho

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks excellent to me, thakns @ranma42!

Right now if this gets past bors then we unfortunately don't have many guarantees. It should exercise all xz-producing code but doesn't exercise the manifest generator, that'll happen when the next nightly is built.

You can also run everything locally (at least container-wise) with ./src/ci/docker/run.sh arm-android (etc)

};
let xz_filename = filename.replace(".tar.gz", ".tar.xz");
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't currently validate that the xz file actually exists, could that be done as an extra sanity check?

Copy link
Contributor Author

@ranma42 ranma42 Apr 28, 2017

Choose a reason for hiding this comment

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

self.digests should only contain digests for existing files. Do you expect the file to be deleted between the digest computation and the construction of the manifest?

Copy link
Member

Choose a reason for hiding this comment

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

Oh good point yeah, but that means that the url could be specified and the digest could be none, right? Could we statically prevent a situation such as that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xz_url: xz_digest.as_ref().map(|_| self.url(&xz_filename)) makes sure that the url is only specified if the digest is Something (and that its contents are based on the filename, not on the digest value).

Copy link
Member

Choose a reason for hiding this comment

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

gah sorry, yes of course!

@@ -11,6 +11,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
cmake \
sudo \
gdb \
p7zip-full \
Copy link
Member

Choose a reason for hiding this comment

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

A bunch of containers don't actually produce tarballs, only the dist-* ones do I believe. Could this package be left out of all the containers (like this one) not producing tarballs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm... Actually I might have removed one too many: does the cross image also produce tarballs? Even if it is not dist-*, it is flagged DEPLOY=1.

Copy link
Member

Choose a reason for hiding this comment

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

Oops yes sorry DEPLOY=1 is the marker to look for.

@alexcrichton
Copy link
Member

cc @brson

@mattico
Copy link
Contributor

mattico commented Apr 28, 2017

Out of curiosity: why are you using p7zip rather than xz-utils (which is already being installed)?

@ranma42
Copy link
Contributor Author

ranma42 commented Apr 28, 2017

@mattico 7z is available on AppVeyor (where it is somewhat harder to install software). See rust-lang/rust-installer#57 (comment)

@alexcrichton
Copy link
Member

Ok everything looks great to me @ranma42! Want to squash and remove '[WIP]' and r+?

@ranma42 ranma42 changed the title [WIP] Generate XZ-compressed tarballs Generate XZ-compressed tarballs Apr 29, 2017
@ranma42
Copy link
Contributor Author

ranma42 commented Apr 29, 2017

Squashed and renamed :)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 30, 2017

📌 Commit 36b9ab7 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 30, 2017

⌛ Testing commit 36b9ab7 with merge 5616298...

@bors
Copy link
Contributor

bors commented Apr 30, 2017

💔 Test failed - status-travis

@ranma42
Copy link
Contributor Author

ranma42 commented Apr 30, 2017

Uh, of course distcheck also requires 7z. I amended the last commit adding it (and mentioning in the commit message the affected docker images).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 30, 2017

📌 Commit bf4082a has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 3, 2017

⌛ Testing commit 6bf71f0 with merge d5948bd...

@bors
Copy link
Contributor

bors commented May 3, 2017

💔 Test failed - status-travis

@TimNN
Copy link
Contributor

TimNN commented May 3, 2017

Looks legitimate:

[00:03:47] make-tarballs: error: need 7z

(https://travis-ci.org/rust-lang/rust/jobs/228215768)

@bors r-

MacOSX does not ship `7z` nor `xz`. Let's use `xz`, just like on the
other *nix systems.
@ranma42
Copy link
Contributor Author

ranma42 commented May 3, 2017

I must have messed up when squashing the update of the rust-installer submodule, sorry. It should be fixed by my latest push.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 3, 2017

📌 Commit 5e522d7 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 3, 2017
Generate XZ-compressed tarballs

Integrate the new `rust-installer` and extend manifests with keys for xz-compressed tarballs.

One of the steps required for rust-lang#21724
@bors
Copy link
Contributor

bors commented May 3, 2017

⌛ Testing commit 5e522d7 with merge 234472d...

@aidanhs
Copy link
Member

aidanhs commented May 3, 2017

Think this caused a rollup failure in #41725 on the osx xcode 7 builders

The command "brew update && brew install xz" failed and exited with 1 during

Can't decipher why from the logs, but both of the osx xcode 7 builders failed, so I suspect it'll fail here as well.

@alexcrichton
Copy link
Member

@aidanhs oddly enough all builders have gotten past the xz installation stage here ...

@alexcrichton
Copy link
Member

@bors: r-

er I believe @frewsxcv is correct

@ranma42 can you ensure that there are retry loops around the installations on OSX?

@alexcrichton
Copy link
Member

@bors: retry

Wrap the installation on macOS with `travis_retry`.
@ranma42
Copy link
Contributor Author

ranma42 commented May 3, 2017

I added travis_retry to the update and install commands, but it is not clear to me why the log was mangled in that way (as if two commands were running concurrently).

@alexcrichton
Copy link
Member

@bors: r+

Yeah I also found that log... interestingly ordered

@bors
Copy link
Contributor

bors commented May 3, 2017

📌 Commit 98dd82c has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 3, 2017
Generate XZ-compressed tarballs

Integrate the new `rust-installer` and extend manifests with keys for xz-compressed tarballs.

One of the steps required for rust-lang#21724
bors added a commit that referenced this pull request May 3, 2017
Rollup of 6 pull requests

- Successful merges: #41543, #41600, #41715, #41720, #41721, #41730
- Failed merges:
@bors
Copy link
Contributor

bors commented May 4, 2017

⌛ Testing commit 98dd82c with merge b16c7a2...

@bors bors merged commit 98dd82c into rust-lang:master May 4, 2017
brandt added a commit to brandt/homebrew-core that referenced this pull request May 6, 2017
As of rust-lang/rust#41600 we now need `xz` to build Rust from HEAD.
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.

None yet