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

Add commit information to roc versions built from source #7046

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

crates/cli/src/main.rs Show resolved Hide resolved
crates/cli/src/lib.rs Show resolved Hide resolved
crates/cli/src/main.rs Show resolved Hide resolved
@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
crates/cli/Cargo.toml Outdated Show resolved Hide resolved
crates/cli/build.rs Outdated Show resolved Hide resolved
@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
crates/cli/src/main.rs Outdated Show resolved Hide resolved
crates/cli/build.rs Outdated Show resolved Hide resolved
crates/cli/build.rs Outdated Show resolved Hide resolved
@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
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