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

Show the real commit hash for `./servo --version`, not the bundle hash #26720

Merged
merged 4 commits into from Jun 6, 2020

Conversation

@camelid
Copy link
Contributor

camelid commented May 31, 2020

Show the real commit hash of the build when run on a bundle commit, rather than showing the bundle's hash.

It gets the real commit hash by extracting it from the bundle commit message, which has the form Shallow version of commit {sha1}, where {sha1} is the real commit hash.


  • ./mach build -d does not report any errors (edits Python code, no Rust changes)
  • ./mach test-tidy does not report any errors
  • These changes fix #26386 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because this only changes infrastructure
It's extracted from the commit message of the bundle.
@highfive
Copy link

highfive commented May 31, 2020

Heads up! This PR modifies the following files:

@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

I can't check to see if these changes work because I don't have access to the CI, so let me know if anything seems off!

@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

Oops, this code will fail when you run ./mach build on a non-bundle commit.

@camelid camelid marked this pull request as draft May 31, 2020
camelid added 2 commits May 31, 2020
@camelid camelid marked this pull request as ready for review May 31, 2020
@jdm
Copy link
Member

jdm commented May 31, 2020

@highfive highfive assigned SimonSapin and unassigned Manishearth May 31, 2020
@SimonSapin
Copy link
Member

SimonSapin commented May 31, 2020

@bors-servo try=linux

@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2020

Trying commit e362538 with merge 7ed2432...

bors-servo added a commit that referenced this pull request May 31, 2020
Show the real commit hash for `./servo --version`, not the bundle hash

<!-- Please describe your changes on the following line: -->
Show the real commit hash of the build when run on a bundle commit, rather than showing the bundle's hash.

It gets the real commit hash by extracting it from the bundle commit message, which has the form `Shallow version of commit {sha1}`, where `{sha1}` is the real commit hash.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors (edits Python code, no Rust changes)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26386 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because this only changes infrastructure

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2020

💔 Test failed - status-taskcluster

@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

Weird... it seemed to work fine on my machine. Must be a Python version incompatibility with byte strings vs regular strings or something like that. Sigh.

The error:

TypeError: startswith first arg must be bytes or a tuple of bytes, not str

@SimonSapin
Copy link
Member

SimonSapin commented May 31, 2020

Rigth, this line

https://community-tc.services.mozilla.com/tasks/HdD7jA0MSSiXnhRyEJti6A/runs/0/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FHdD7jA0MSSiXnhRyEJti6A%2Fruns%2F0%2Fartifacts%2Fpublic%2Flogs%2Flive.log#L462

Shows that the error happens with Python 3.

We’re in the middle of the transition #23607, so IIRC most of mach needs to support both 2.x and 3.x at the moment.

git_commit_subject is the result of subprocess.check_output and so a byte string. In 2.x those are freely interchangeable with Unicode strings as long as the contents is ASCII. "Shallow version of commit " is a Unicode string in 3.x. You can make it a byte string (in both) by adding a b prefix before the opening quote. An similarly for the ' ' string passed to split in the next non-comment line.

@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

Ah, I see. I guess I should run mach with Python 3 on my system so that I catch those breaking changes :)

@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

It seems that the version is only updated if the config crate is changed or you do a fresh build. Is there a way to fix that?

@SimonSapin
Copy link
Member

SimonSapin commented May 31, 2020

It looks like mach sets the GIT_INFO environment variable, and components/config/lib.rs uses option_env!("GIT_INFO") to read it. rustc knows about this, but apparently Cargo doesn’t.

We could potentially fix this by adding a build script that prints cargo:rerun-if-env-changed=GIT_INFO: https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorerun-if-env-changedname

The problem is that this seems to disable the default dependency tracking: rust-lang/cargo#4587 so we’d need to also print cargo:rerun-if-changed=lib.rs and similarly for each source file. And that code could easily get out of sync with the actual set of source files.

However there is ongoing work in rustc rust-lang/rust#71858 to expose data about dependencies on environment variables. Cargo can eventually read that data and do the right thing.

So, given that this will be fixed upstream eventually, I think it’s not worth doing the build script work-around. In the meantime you can run touch components/config/lib.rs to force a rebuild.

@SimonSapin
Copy link
Member

SimonSapin commented May 31, 2020

@bors-servo try=linux

@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2020

Trying commit 2e5ad99 with merge 3bf3e59...

bors-servo added a commit that referenced this pull request May 31, 2020
Show the real commit hash for `./servo --version`, not the bundle hash

<!-- Please describe your changes on the following line: -->
Show the real commit hash of the build when run on a bundle commit, rather than showing the bundle's hash.

It gets the real commit hash by extracting it from the bundle commit message, which has the form `Shallow version of commit {sha1}`, where `{sha1}` is the real commit hash.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors (edits Python code, no Rust changes)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26386 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because this only changes infrastructure

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@camelid
Copy link
Contributor Author

camelid commented May 31, 2020

In the meantime you can run touch components/config/lib.rs to force a rebuild.

Could we just have mach run touch? I guess the only issue is then it would rebuild that part every time. Maybe just for release builds?

@SimonSapin
Copy link
Member

SimonSapin commented May 31, 2020

I would not be in favor:

  • Like the build script, this is another kind of work-around that we’d have to maintain
  • The proper fix is coming to rustc + cargo
  • The consequences of this bug are extremely mild: CI (including Nightly builds) is not affected since it only builds once, and for local builds made by contributors the output of --version is not that important in my opinion.
@bors-servo
Copy link
Contributor

bors-servo commented May 31, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True

@camelid
Copy link
Contributor Author

camelid commented Jun 6, 2020

@SimonSapin could you review this? Thanks!

@SimonSapin
Copy link
Member

SimonSapin commented Jun 6, 2020

try builds don’t upload build artifacts that I could test, so I hacked together a build with the same configuration as Nightly at https://community-tc.services.mozilla.com/tasks/L5hH2dsmQ7CvGRXv-G5-fA#artifacts and verified that ./servo --version prints Servo 0.0.1-18a73cef2 which matches commit 18a73ce that I pushed.

In short, looks good, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2020

📌 Commit 2e5ad99 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2020

Testing commit 2e5ad99 with merge 51fd0e8...

@camelid
Copy link
Contributor Author

camelid commented Jun 6, 2020

Thank you! :)

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2020

☀️ Test successful - status-taskcluster
Approved by: SimonSapin
Pushing 51fd0e8 to master...

@bors-servo bors-servo merged commit 51fd0e8 into servo:master Jun 6, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.