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

feat: Add git info to build #4804

Merged
merged 15 commits into from
Aug 5, 2024
Merged

feat: Add git info to build #4804

merged 15 commits into from
Aug 5, 2024

Conversation

max-sixty
Copy link
Member

@max-sixty max-sixty commented Aug 1, 2024

Not finished; needs some cleaning + need to resolve how the tests & book should work Ready to merge

Would replace #4546 (hope that's OK!)

@max-sixty
Copy link
Member Author

This requires bumping the MSRV from 1.70.0 to 1.73.0. Would that be OK with everyone? @eitsupi ?

Debian is now on 1.79.0. 1.73.0 is 10 months old.

@vanillajonathan
Copy link
Collaborator

Bumping MSRV is fine.
Note that using fetch-tags: true may make the repo cloning take a longer time.

@eitsupi
Copy link
Member

eitsupi commented Aug 3, 2024

Thanks for working on this.
I think this feature is great and setting the MSRV to 1.73 is fine, but I don't know if this design is reasonable.
Do you know of any projects that would be helpful?

@max-sixty
Copy link
Member Author

max-sixty commented Aug 3, 2024

Do you know of any projects that would be helpful?

The benefit is to give us an exact version for reproducing issues. For example, if I launch a playground on this code, I get -- Generated by PRQL compiler version:0.13.0-31-gfa46187 (https://prql-lang.org), rather than 0.13.1, which isn't quite correct.

edit: I think probably I misunderstood, given you also said "I think this feature is great" — can I ask what you mean by " Do you know of any projects that would be helpful?"? TY!

max-sixty added a commit to max-sixty/prql that referenced this pull request Aug 3, 2024
@max-sixty
Copy link
Member Author

I switched to using gitcl, because of rustyhorde/vergen#359. That uses the git command line — a bit less elegant but OK for the moment.

I would have thought it would be a bit slower, but actually the time to execute time bash -c 'touch prqlc/prqlc/src/lib.rs; cargo build -p prqlc' isn't noticeable. (Possibly because there's lots of noise already in that timing — anything from 1.8s to 2.4s...)

time bash -c 'touch prqlc/prqlc/src/lib.rs; cargo build -p prqlc'
   Compiling prqlc v0.13.1 (/Users/maximilian/workspace/prql/prqlc/prqlc)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.90s

________________________________________________________
Executed in    1.95 secs    fish           external
   usr time    1.55 secs   65.00 micros    1.55 secs
   sys time    2.29 secs  371.00 micros    2.29 secs

@eitsupi
Copy link
Member

eitsupi commented Aug 4, 2024

can I ask what you mean by " Do you know of any projects that would be helpful?"? TY!

Sorry for failing to ask the right question. I just wanted to know if there are other packages out there with similar functionality.

@max-sixty
Copy link
Member Author

Sorry for failing to ask the right question. I just wanted to know if there are other packages out there with similar functionality.

Ah, sorry, I understand now!

Lots of python projects do this — so running xarray.__version__ gives a git hash when it's not on a tag, for example.

For rust — I browsed dependents of vergen, which has 20K repos and 200 packages. GitHub doesn't organize by popularity, but one random example is https://github.com/sxyazi/yazi/blob/d2ebadb6314e3d124a173b6683399e8d33acb429/yazi-cli/build.rs#L8

@max-sixty max-sixty merged commit eb62df9 into PRQL:main Aug 5, 2024
85 of 86 checks passed
@max-sixty max-sixty deleted the build-info branch August 5, 2024 16:41
max-sixty added a commit to max-sixty/prql that referenced this pull request Aug 5, 2024
max-sixty added a commit that referenced this pull request Aug 5, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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