Skip to content

Add full git commit hash to release channel manifests #44218

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

Merged
merged 4 commits into from
Sep 7, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/bootstrap/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,9 @@ impl Step for Rustc {
cp("README.md");
// tiny morsel of metadata is used by rust-packaging
let version = build.rust_version();
let sha = build.rust_sha().unwrap_or("");
Copy link
Member

Choose a reason for hiding this comment

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

<unknown> seems better to me than an empty string, but I'm not sure if it'll work well with other logic. Perhaps we could not create the file if we don't know it instead? Empty files are somewhat painful to debug...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this omit the file at first, but then I didn’t know how to separate “this tarball does not contain this file” from other kinds of errors in Command::new("tar").arg("xf") …

Any suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it should be an error to try to build a manifest with ignore-git = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to not including the file when the info is missing.

t!(t!(File::create(overlay.join("version"))).write_all(version.as_bytes()));
t!(t!(File::create(overlay.join("git-commit-hash"))).write_all(sha.as_bytes()));
Copy link
Member

Choose a reason for hiding this comment

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

This version file is currently present in a number of tarballs that we produce, and from a consistency point of view it seems odd for the hash to only be in one? Would it be possible to refactor this to ensure there's a git commit file in all the tarballs?

Copy link
Contributor Author

@SimonSapin SimonSapin Sep 1, 2017

Choose a reason for hiding this comment

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

Added for source and extended tarballs. I don’t know if the rust-lang/rust commit hash makes sense in Cargo and RLS tarballs, and the rust-lang/cargo and rust-lang-nursery/rls commit hashes are not easily available.


// On MinGW we've got a few runtime DLL dependencies that we need to
// include. The first argument to this script is where to put these DLLs
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,11 @@ impl Build {
self.rust_info.version(self, channel::CFG_RELEASE_NUM)
}

/// Return the full commit hash
fn rust_sha(&self) -> Option<&str> {
self.rust_info.sha()
}

/// Returns the `a.b.c` version that the given package is at.
fn release_num(&self, package: &str) -> String {
let mut toml = String::new();
Expand Down
53 changes: 33 additions & 20 deletions src/tools/build-manifest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ static MINGW: &'static [&'static str] = &[
struct Manifest {
manifest_version: String,
date: String,
git_commit_hash: String,
Copy link
Member

Choose a reason for hiding this comment

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

I think that this commit may be best inside each Package? I'd imagine that maybe eventually we'd have a use for the commit information other than just for rustc but also for cargo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything under https://s3.amazonaws.com/rust-lang-ci/rustc-builds are in "subdirectories" named after the rust-lang/rust commit hash, even for other packages like Cargo. But if you prefer I could work on that change in case the Cargo commit hash is needed later for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figuring out the respective build systems in respective repositories to add text files in various tarballs to make this info available in the first place is non-trivial, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve made it optional (always missing at the moment for Cargo and RLS) and per-package.

pkg: BTreeMap<String, Package>,
}

Expand Down Expand Up @@ -205,15 +206,10 @@ impl Builder {

self.digest_and_sign();
let manifest = self.build_manifest();
let filename = format!("channel-rust-{}.toml", self.rust_release);
self.write_manifest(&toml::to_string(&manifest).unwrap(), &filename);

let filename = format!("channel-rust-{}-date.txt", self.rust_release);
self.write_date_stamp(&manifest.date, &filename);
self.write_channel_files(&self.rust_release, &manifest);

if self.rust_release != "beta" && self.rust_release != "nightly" {
self.write_manifest(&toml::to_string(&manifest).unwrap(), "channel-rust-stable.toml");
self.write_date_stamp(&manifest.date, "channel-rust-stable-date.txt");
self.write_channel_files("stable", &manifest);
}
}

Expand All @@ -230,6 +226,7 @@ impl Builder {
let mut manifest = Manifest {
manifest_version: "2".to_string(),
date: self.date.to_string(),
git_commit_hash: self.git_commit_hash("rust", "x86_64-unknown-linux-gnu"),
pkg: BTreeMap::new(),
};

Expand Down Expand Up @@ -382,14 +379,31 @@ impl Builder {
.arg(self.input.join(&filename))
.arg(format!("{}/version", filename.replace(".tar.gz", "")))
.arg("-O");
let version = t!(cmd.output());
if !version.status.success() {
let output = t!(cmd.output());
if !output.status.success() {
panic!("failed to learn version:\n\n{:?}\n\n{}\n\n{}",
cmd,
String::from_utf8_lossy(&version.stdout),
String::from_utf8_lossy(&version.stderr));
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr));
}
String::from_utf8_lossy(&output.stdout).trim().to_string()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm not quite following, but why the changes to version()? It still should be doing the same thing, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed a variable name before copy/pasting the method, but it’s otherwise unchanged. I can revert that if you prefer.

}

fn git_commit_hash(&self, component: &str, target: &str) -> String {
let mut cmd = Command::new("tar");
let filename = self.filename(component, target);
cmd.arg("xf")
.arg(self.input.join(&filename))
.arg(format!("{}/git-commit-hash", filename.replace(".tar.gz", "")))
.arg("-O");
let output = t!(cmd.output());
if !output.status.success() {
panic!("failed to learn git commit hash:\n\n{:?}\n\n{}\n\n{}",
cmd,
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr));
}
String::from_utf8_lossy(&version.stdout).trim().to_string()
String::from_utf8_lossy(&output.stdout).trim().to_string()
}

fn hash(&self, path: &Path) -> String {
Expand Down Expand Up @@ -425,16 +439,15 @@ impl Builder {
assert!(t!(child.wait()).success());
}

fn write_manifest(&self, manifest: &str, name: &str) {
let dst = self.output.join(name);
t!(t!(File::create(&dst)).write_all(manifest.as_bytes()));
self.hash(&dst);
self.sign(&dst);
fn write_channel_files(&self, channel_name: &str, manifest: &Manifest) {
self.write(&toml::to_string(&manifest).unwrap(), channel_name, ".toml");
self.write(&manifest.date, channel_name, "-date.txt");
self.write(&manifest.git_commit_hash, channel_name, "-git-commit-hash.txt");
Copy link
Member

Choose a reason for hiding this comment

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

Do we for sure need a dedicated file for the git commit? This'll already be present in the manifest, and the date.txt was only added much later as a request (ideally this extra file wouldn't exist)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don’t really need it, just like we don’t really need date.txt. A separate file makes it marginally easier for Servo’s bootstrap to extract the commit hash, but we can also parse the TOML file.

I can remove it if you prefer.

}

fn write_date_stamp(&self, date: &str, name: &str) {
let dst = self.output.join(name);
t!(t!(File::create(&dst)).write_all(date.as_bytes()));
fn write(&self, contents: &str, channel_name: &str, suffix: &str) {
let dst = self.output.join(format!("channel-rust-{}{}", channel_name, suffix));
t!(t!(File::create(&dst)).write_all(contents.as_bytes()));
self.hash(&dst);
self.sign(&dst);
}
Expand Down