-
Notifications
You must be signed in to change notification settings - Fork 222
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 build information to build comment #4546
Conversation
hmm CI tests are failing in an interesting way:
I don't know how Maybe there's something about how the CI is run that I don't understand? Is |
Nice! Interesting re the git issue. I agree the commit doesn't seem to exist. But OTOH it does state here that that does seem to be the commit... https://github.com/PRQL/prql/actions/runs/9395042476/job/25873760775?pr=4546#step:2:56 I think it's probably creating a merge commit to simulate this branch being merged into main. And it's possibly doing a shallow clone so the tags aren't there. Do you know if we can change the clone to be less shallow? Either way we should make this robust to not having tags there — e.g. fall back to an empty string, I think. |
Yes seems like we need something like: actions/checkout#217 (comment) |
No obligation to use vergen, but:
|
let output = Command::new("git") | ||
.args(["describe", "--tags"]) | ||
.output() | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean that the build will fail if the git command is not available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will be a problem on some build platforms without git or if the build process strips git repo out of source code.
I'm not sure how this works with downstream packages. What happens if I install from crates.io? |
It would need to fall back to just the version number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unnecessary for released versions of prqlc, as version number already describes git tag, which gets us the git rev.
It is useful for development builds. If you also think so, I'd suggest we:
- add git commit hash only if current commit does not have a git tag,
- silently skip this if git is not installed,
- silently skip this if git repo is not found.
let output = Command::new("git") | ||
.args(["describe", "--tags"]) | ||
.output() | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will be a problem on some build platforms without git or if the build process strips git repo out of source code.
No urgency, but just FYI I thought this was cool and would be nice to have! |
Adds feat. requested in #3393
example output:
On my M1 pro, it appears to add ~0.5s to the re-build process, bringing the total to somewhere between 3.6 and 5.7 seconds