Skip to content
This repository has been archived by the owner on Nov 21, 2018. It is now read-only.

Build v2 manifests #66

Merged
merged 1 commit into from Mar 8, 2016
Merged

Build v2 manifests #66

merged 1 commit into from Mar 8, 2016

Conversation

brson
Copy link
Contributor

@brson brson commented Mar 7, 2016

This builds v2 manifests during finish_dists for the "rust" component. It adds the file build-rust-manifest.py. It's well-commented so read it for details.

This adds a new channel-$component-$channel-date.txt file during finish_dist as part of v1 manifest generation. Then, when building the "rust" component, build-rust-manifest discovers the cargo and rustc archive dates from that file on s3. It then validates the existence of the cargo and rustc components by pinging s3, and the final rust components by looking at the local disk.

Two ugly points to note:

r? @alexcrichton cc @edunham

I'd like to deploy this tonight.

@brson
Copy link
Contributor Author

brson commented Mar 7, 2016

Example.

@brson brson mentioned this pull request Mar 7, 2016
Closed
manifest_v2_name = "channel-" + component + "-" + channel + ".toml"
manifest_v2_file = final_dist_dir + "/" + manifest_v2_name
# FIXME: This is a poor way to find the build-rust-manifest script
manifest_v2_cmd = ("python $HOME/rust-buildbot/master/build-rust-manifest.py " +
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% privy to how python works, but couldn't this be import build_rust_manifest at the top of this file? That can import the local module directly, right? That may also help in marshalling arguments back and forth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible. The reasons I didn't are:

  • Assigning the arguments to globals in the python script is nice. I did spend a few minutes threading them around and decided I didn't want to.
  • I was hopeful that running this as a MasterShellCommand would automatically do it asynchronously on another thread. Blocking here would be bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using globals will be unpleasant if we ever attempt to unit test the script. Based on what I've learned from attempting to do the same thing earlier, I think having the script separate may make it easier to add debugging information and tests in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh blocking is a killer point, disregard me!

target_list = sorted([
"aarch64-unknown-linux-gnu",
"arm-linux-androideabi",
"arm-unknown-linux-gnueabif",
Copy link
Contributor

Choose a reason for hiding this comment

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

"gnueabi"

@brson brson force-pushed the wip branch 2 times, most recently from 07d6b56 to 7bae73a Compare March 7, 2016 23:28
"x86_64-pc-windows-msvc",
"x86_64-unknown-linux-gnu",
"x86_64-unknown-linux-musl",
])
Copy link
Contributor

Choose a reason for hiding this comment

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

  • aarch64-apple-ios
  • armv7-apple-ios
  • armv7-unknown-linux-gnueabihf
  • armv7s-apple-ios
  • i386-apple-ios
  • powerpc-unknown-linux-gnu
  • powerpc64-unknown-linux-gnu
  • powerpc64le-unknown-linux-gnu
  • x86_64-apple-ios
  • x86_64-rumprun-netbsd

Copy link
Contributor

Choose a reason for hiding this comment

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

er, meant to prefix that with "Some other targets":

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a list of these kinda in master.cfg which we'll need to keep in sync, but probably too difficult for now to refactor

@alexcrichton
Copy link
Contributor

A few blocking comments in master.cfg, but otherwise r=me for the new script and with those changes applied

installer_file = temp_dir + "/" + installer_name
f = open(installer_file, "w")
while True:
buf = response.read(4096)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tarball could get corrupted during download; is it worth checking the shasum or trying to find a local copy of the tarball (as I believe there will be for rustc though probably not cargo) first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be corrupted, as could other downloads here. There aren't going to be a local rustc build available at the time this is run though without other changes to rust-buildbot. This step at least could verify the shas. At least in this case if the tarball is corrupted it probably won't unpack. If the shas are corrupted on download this script will possibly just insert garbage into the manifest.

@brson
Copy link
Contributor Author

brson commented Mar 8, 2016

This script has a race condition on its temporary directory when run in parallel. It's unlikely but will probably happen eventually. I think I'll add the temp directory as an argument and make them different per channel.

Conflicts:
	master/master.cfg
@brson
Copy link
Contributor Author

brson commented Mar 8, 2016

I've updated to address feedback. I have not changed the way tar is invoked, since this way is working, nor consolidated the platform lists, or taken any additional steps to verify downloads.

brson added a commit that referenced this pull request Mar 8, 2016
@brson brson merged commit b2b34a2 into rust-lang-deprecated:master Mar 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants