Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd `--version` flag #11262
Merged
Add `--version` flag #11262
Conversation
highfive
commented
May 19, 2016
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. |
highfive
commented
May 19, 2016
|
Heads up! This PR modifies the following files:
|
|
Could we also include the commit hash ( |
|
Please do not include the BUILD_DATE - this kills reproducibility. We could use the SOURCE_DATE_EPOCH environment variable instead, but I'd rather just not have the timestamp at all. |
|
Oh, good point. I'd prefer to just have a git hash in that case. |
|
git hash works for me. |
|
I agree that including the build date is problematic. The build date has no real correlation with version (I could check out a really old version and build today for example). It might be better to include the git hash instead of the date, rather than in addition to. |
|
If you are going to include a hash, it might be nice to include whether the tree was dirty when built. This has been very useful in the Opus and Daala projects to let us know when bug reports come in with modifications from the source tree. See https://github.com/xiph/daala/search?utf8=%E2%9C%93&q=dirty for example. |
|
Here is all the different data that can be easily accessed using this PR as an example:
Right now the only tag that exists is So given all of that as possible input, is there a consensus on what the output of |
|
We don't use tags yet, perhaps |
|
Should that also include an optional |
|
+1 on optional dirty suffix. Also, please make the git-related info optional; this is much nicer for packaging because you can build from a tarball instead of needing the full git history (or a shallow clone). (I imagine that in the future, |
highfive
commented
May 22, 2016
|
New code was committed to pull request. |
|
The updated CR has the following behavior:
I moved the logic for printing the version into The only behavior that I'm unsure of is what to do when changes occur outside of source files. For example, if I build with uncommitted changes, then commit those changes, then try to build again nothing will happen because no rust file has changed, and the servo binary will still say |
|
|
|
ping @Manishearth! |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Jun 9, 2016
Add `--version` flag - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #11241 (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ Not 100% sure of a good way to test this, so I'm submitting as is for feedback. Manually testing it appears to work fine. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11262) <!-- Reviewable:end -->
|
|
highfive
commented
Jun 9, 2016
|
components/servo/main.rs
Outdated
| use std::rc::Rc; | ||
|
|
||
| fn main() { | ||
| // Parse the command line options and store them globally | ||
| let opts_result = opts::from_cmdline_args(&*args()); | ||
|
|
||
| if opts::get().is_printing_version { |
This comment has been minimized.
This comment has been minimized.
Ms2ger
Jun 9, 2016
Contributor
Move this below the if let Some(token) = content_process_token { block below.
|
Any update? |
|
Sorry, I've been really busy. I'll make the requested change tonight and update the PR. |
|
@bors-servo: r=Manishearth |
|
|
|
|
bors-servo
added a commit
that referenced
this pull request
Jun 29, 2016
Add `--version` flag - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #11241 (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ Not 100% sure of a good way to test this, so I'm submitting as is for feedback. Manually testing it appears to work fine. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11262) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
campaul commentedMay 19, 2016
•
edited by larsbergstrom
./mach build -ddoes not report any errors./mach test-tidy --fasterdoes not report any errorsEither:
Not 100% sure of a good way to test this, so I'm submitting as is for feedback. Manually testing it appears to work fine.
This change is