Add full git commit hash to release channel manifests #44218

SimonSapin · 2017-08-31T14:39:24Z

The full hash is necessary to build the download URL for "alternate" compiler builds. This is a first step for rust-lang/rustup#1099.

The full hash is necessary to build the download URL for "alternate" compiler builds. This is a first step for rust-lang/rustup#1099

rust-highfive · 2017-08-31T14:39:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

alexcrichton · 2017-08-31T20:39:24Z

src/bootstrap/dist.rs

        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()));


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?

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.

alexcrichton · 2017-08-31T20:41:17Z

src/tools/build-manifest/src/main.rs

+    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");


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)

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.

alexcrichton · 2017-08-31T20:42:10Z

src/tools/build-manifest/src/main.rs

@@ -107,6 +107,7 @@ static MINGW: &'static [&'static str] = &[
 struct Manifest {
    manifest_version: String,
    date: String,
+    git_commit_hash: String,


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

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.

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.

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

Mark-Simulacrum

This seems fine in general but there are a few implementation nits. Others can comment more on the rustup/artifact side.

Mark-Simulacrum · 2017-08-31T17:13:35Z

src/bootstrap/dist.rs

@@ -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("");


<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...

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?

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

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

Mark-Simulacrum · 2017-08-31T22:52:27Z

src/tools/build-manifest/src/main.rs

+                   String::from_utf8_lossy(&output.stdout),
+                   String::from_utf8_lossy(&output.stderr));
+        }
+        String::from_utf8_lossy(&output.stdout).trim().to_string()


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

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

… instead of writing an empty file.

alexcrichton · 2017-09-05T16:22:51Z

src/tools/build-manifest/src/main.rs

@@ -167,6 +168,9 @@ struct Builder {
    rust_version: String,
    cargo_version: String,
    rls_version: String,
+    rust_git_commit_hash: Option<String>,
+    cargo_git_commit_hash: Option<String>,


It looks like these are never initialized?

alexcrichton · 2017-09-05T16:23:40Z

Looks good! I think though that cargo/rls tarballs don't have files with the git commit in them?

At the moment it is always missing for Cargo and RLS. Their respective build systems need to be modified to include `git-commit-hash` files in their "dist" tarballs.

SimonSapin · 2017-09-05T17:40:50Z

I think though that cargo/rls tarballs don't have files with the git commit in them?

That’s correct. These would need to be added separately. (If someone finds a use for them, they are not used for rust-lang/rustup#1099 which motivates this PR.)

alexcrichton · 2017-09-05T18:59:24Z

@bors: r+

bors · 2017-09-05T18:59:25Z

📌 Commit f912d77 has been approved by alexcrichton

Mark-Simulacrum · 2017-09-06T16:47:39Z

@bors rollup

Add full git commit hash to release channel manifests The full hash is necessary to build the download URL for "alternate" compiler builds. This is a first step for rust-lang/rustup#1099.

Rollup of 23 pull requests - Successful merges: #44097, #44206, #44218, #44276, #44277, #44296, #44303, #44313, #44315, #44317, #44319, #44321, #44325, #44326, #44327, #44328, #44330, #44351, #44353, #44354, #44361, #44362, #44377 - Failed merges:

… added in rust-lang/rust#44218, instead of using the GitHub API. And upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07)

Get rustc commit hash from channel manifest … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07), and app_units 0.5.6 (for servo/app_units#37)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18420)

… added in rust-lang/rust#44218, instead of using the GitHub API. And upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07)

Get rustc commit hash from channel manifest … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07), and app_units 0.5.6 (for servo/app_units#37)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18420)

… added in rust-lang/rust#44218, instead of using the GitHub API.

Get rustc commit hash from channel manifest … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18420)

… added in rust-lang/rust#44218, instead of using the GitHub API.

Get rustc commit hash from channel manifest … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18420)

…om servo:toml); r=nox,emilio … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07). Source-Repo: https://github.com/servo/servo Source-Revision: af077a722225193b38d80622fb939b7719b46db0 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 4ec174151648685b4eb0204ec8798d4f82fde9c9

…om servo:toml); r=nox,emilio … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07). Source-Repo: https://github.com/servo/servo Source-Revision: af077a722225193b38d80622fb939b7719b46db0

…om servo:toml); r=nox,emilio … added in rust-lang/rust#44218, instead of using the GitHub API. Also upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07). Source-Repo: https://github.com/servo/servo Source-Revision: af077a722225193b38d80622fb939b7719b46db0 UltraBlame original commit: b674d9f6d1d65fba6b68cb847619d1c7d84c0d4c

Add full git commit hash to release channel manifests

de038b7

The full hash is necessary to build the download URL for "alternate" compiler builds. This is a first step for rust-lang/rustup#1099

rust-highfive assigned alexcrichton Aug 31, 2017

SimonSapin mentioned this pull request Aug 31, 2017

Add support for a nightly-alt toolchain rust-lang/rustup#1099

Closed

alexcrichton reviewed Aug 31, 2017

View reviewed changes

Mark-Simulacrum reviewed Aug 31, 2017

View reviewed changes

Add git-commit-hash in source and extended tarballs too.

9abc549

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2017

Only include git-commit-hash in tarballs when available

9412fd7

… instead of writing an empty file.

alexcrichton reviewed Sep 5, 2017

View reviewed changes

Make git commit info optional and per-package in channel manifests

f912d77

At the moment it is always missing for Cargo and RLS. Their respective build systems need to be modified to include `git-commit-hash` files in their "dist" tarballs.

SimonSapin force-pushed the commit-hash branch from e371f53 to f912d77 Compare September 5, 2017 17:38

Mark-Simulacrum mentioned this pull request Sep 6, 2017

Rollup of 22 pull requests #44369

Closed

Mark-Simulacrum mentioned this pull request Sep 7, 2017

Rollup of 23 pull requests #44380

Merged

bors merged commit f912d77 into rust-lang:master Sep 7, 2017

SimonSapin added a commit to servo/servo that referenced this pull request Sep 8, 2017

Get rustc commit hash from channel manifest

3c0680a

… added in rust-lang/rust#44218, instead of using the GitHub API. And upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07)

SimonSapin mentioned this pull request Sep 8, 2017

Get rustc commit hash from channel manifest servo/servo#18420

Merged

SimonSapin added a commit to servo/servo that referenced this pull request Sep 9, 2017

Get rustc commit hash from channel manifest

64eb2e0

… added in rust-lang/rust#44218, instead of using the GitHub API. And upgrade to rustc 1.22.0-nightly (d93036a04 2017-09-07)

SimonSapin added a commit to servo/servo that referenced this pull request Sep 12, 2017

Get rustc commit hash from channel manifest

a71fe3c

… added in rust-lang/rust#44218, instead of using the GitHub API.

SimonSapin added a commit to servo/servo that referenced this pull request Sep 12, 2017

Get rustc commit hash from channel manifest

f59bcc0

… added in rust-lang/rust#44218, instead of using the GitHub API.

SimonSapin deleted the commit-hash branch November 16, 2017 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full git commit hash to release channel manifests #44218

Add full git commit hash to release channel manifests #44218

SimonSapin commented Aug 31, 2017

rust-highfive commented Aug 31, 2017

alexcrichton Aug 31, 2017

SimonSapin Sep 1, 2017 •

edited

Loading

alexcrichton Aug 31, 2017

SimonSapin Sep 1, 2017

alexcrichton Aug 31, 2017

SimonSapin Sep 1, 2017

SimonSapin Sep 1, 2017

SimonSapin Sep 4, 2017

Mark-Simulacrum left a comment

Mark-Simulacrum Aug 31, 2017

SimonSapin Sep 1, 2017

SimonSapin Sep 1, 2017

SimonSapin Sep 4, 2017

Mark-Simulacrum Aug 31, 2017

SimonSapin Sep 1, 2017

alexcrichton Sep 5, 2017

SimonSapin Sep 5, 2017

alexcrichton commented Sep 5, 2017

SimonSapin commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

bors commented Sep 5, 2017

Mark-Simulacrum commented Sep 6, 2017

		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()));

Add full git commit hash to release channel manifests #44218

Add full git commit hash to release channel manifests #44218

Conversation

SimonSapin commented Aug 31, 2017

rust-highfive commented Aug 31, 2017

Choose a reason for hiding this comment

SimonSapin Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 5, 2017

SimonSapin commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

bors commented Sep 5, 2017

Mark-Simulacrum commented Sep 6, 2017

SimonSapin Sep 1, 2017 •

edited

Loading