Skip to content

Add commit information to roc versions built from source#7046

Merged
Anton-4 merged 1 commit intoroc-lang:mainfrom
Hasnep:add-commit-hash-to-version-output
Sep 26, 2024
Merged

Add commit information to roc versions built from source#7046
Anton-4 merged 1 commit intoroc-lang:mainfrom
Hasnep:add-commit-hash-to-version-output

Conversation

@Hasnep
Copy link
Contributor

@Hasnep Hasnep commented Sep 1, 2024

Addresses #7030.

I'm not that experienced with Rust and I'm not familiar with the style and priorities of the Roc compiler team, so I'd appreciate feedback on anything that looks out of place or inefficient 😅

I think this is nearly done, but I'm getting a default commit timestamp when I run it locally:

roc built from commit 6d0a9b69d, committed at 1980-01-01T00:00:00.000000000Z

I think it's a default timestamp, maybe for reproducible builds? But that doesn't make sense to me because it's not a build timestamp, it's a commit timestamp 🤔

Again, any help is appreciated! 😀

Copy link
Contributor Author

@Hasnep Hasnep left a comment

Choose a reason for hiding this comment

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

Just left some comments explaining my changes

@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 2, 2024

Thanks for helping out @Hasnep!

What do you think about calling git using Command?
We like to avoid extra dependencies (like vergen-gitcl) when we can.

@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 4, 2024

What do you think about calling git using Command? We like to avoid extra dependencies (like vergen-gitcl) when we can.

Nice, this is something I considered but wasn't sure about, so I just used a package for the initial version, I'll switch to using Command, thanks :)

@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch from 6d0a9b6 to db485ad Compare September 12, 2024 10:47
@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 12, 2024

@Anton-4 I've removed vergen as a dependency and now directly call the git CLI to get the latest commit's information. The output works as expected now:

❱ cargo run -- --version
roc built from commit db485ad90, committed at 2024-09-12 10:46:45
❱ cargo run -- version
roc built from commit db485ad90, committed at 2024-09-12 10:46:45

and when the repo has changes:

❱ cargo run -- version
roc built from commit db485ad90 with changes, committed at 2024-09-12 10:46:45

Let me know if you think there's anything that I can improve :)

@Hasnep Hasnep marked this pull request as ready for review September 12, 2024 10:54
@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch 2 times, most recently from be27201 to f52697f Compare September 12, 2024 10:57
@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch from a634d67 to 82256b6 Compare September 17, 2024 02:49
@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 17, 2024

Thanks for your comments @Anton-4, I've addressed them all now 👍

Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell left a comment

Choose a reason for hiding this comment

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

This will be really useful. Thank you @Hasnep

@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 17, 2024

To fix the test failures you'll need to do cargo fmt --all and for the nix-build failure I recommend printing git_show_output, perhaps there is some useful stderr about why the timestamp is not there...

@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch 2 times, most recently from 13c3f6a to 80aaf0c Compare September 18, 2024 01:52
@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 19, 2024

The nix-build is failing because Nix filters out .git and other VCS related files from builds to try and make them more reproducible. I've looked a bit into how I can override this behaviour, but Nix is not documented very well :/

@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 22, 2024

but Nix is not documented very well :/

Yes, that is true 😅

I think instead of panicking with "Failed to parse timestamp as an integer" we could default to the string "missing timestamp", that would be most inline with the nix philosophy vs trying to work around this.

@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 25, 2024

I've added a check to see if the .git/HEAD file exists before trying to run any git commands, nix build now runs on my laptop and running the built binary prints roc built from source.

@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch from d672180 to 4170f47 Compare September 25, 2024 13:11
@Hasnep Hasnep force-pushed the add-commit-hash-to-version-output branch from 4170f47 to 058ef10 Compare September 25, 2024 13:13
@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 26, 2024

Thanks for the sustained effort on this @Hasnep ❤️

@Anton-4 Anton-4 merged commit 2d03697 into roc-lang:main Sep 26, 2024
@Hasnep
Copy link
Contributor Author

Hasnep commented Sep 26, 2024

Thanks for the sustained effort on this @Hasnep ❤️

No problem! It was good fun to do my first (proper) contribution to Roc! I'm going to be on the lookout for more issues like this one to attempt that don't require touching the core compiler :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants