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.

Sign up for GitHub

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

Jump to bottom

emit cargo metadata duiring build scripts to avoid outdated buildscript outputs #6743

Merged
merged 2 commits into from
Apr 14, 2023
Merged

emit cargo metadata duiring build scripts to avoid outdated buildscript outputs #6743

merged 2 commits into from
Apr 14, 2023

Conversation

pascalkuthe
Copy link
Member

While triaging issues I often noticed that the revision reported by helix --version if often wrong. I finally found the reason for this: The build.rs script that extracts the HEAD branch by calling git is not automatically rerun so if you build helix then checkout/merge a branch/revision that doesn't touch helix-loader (which doesn't see that much activity compared to the rest of the editor) and rebuild then build script will not be rerun andhelix --version will report the old (incorrect) revision.

I made the build script emit an appropriate cargo:rerun-if-changed= so that it is rerun if HEAD changes to fix that.

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 13, 2023
@pascalkuthe pascalkuthe changed the title rebuild on revision change rebuild on git revision change Apr 13, 2023
the-mikedavis
the-mikedavis previously approved these changes Apr 13, 2023
View reviewed changes
dead10ck
dead10ck requested changes Apr 14, 2023
View reviewed changes
Copy link
Member

@dead10ck dead10ck left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Oh shoot, I had stumbled across this myself a long time ago and then totally forgot to make a PR! I think we also have to rerun if basically any file changes inside runtime/grammars/sources, or the languages.toml file too.

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Apr 14, 2023 •
edited
Loading

I looked into adding this. We are already rebuilding on languages.toml changes since we include_bytes! those which is a compiler builtin that handles invalidation correctly.

I added the metadata for runtime/grammars/sources. In the process I noticed that the simple approach I had taken to git metadata before didn't quite work and I didn't consider that HEAD is (usually) a git symbolic-ref and that .git can also be a reference (for detached work trees which I am even using myself). Considering that I heavily contribute to gitoxide I really should know better but it slipped me by. The new solution is much more robust. Sorry about that.

This probably needs a fresh review as a result

@pascalkuthe pascalkuthe changed the title rebuild on git revision change emit correct cargo metadata duiring build scripts to avoid out of state buildscript results Apr 14, 2023
@pascalkuthe pascalkuthe changed the title emit correct cargo metadata duiring build scripts to avoid out of state buildscript results emit cargo metadata duiring build scripts to avoid outdated buildscript outputs Apr 14, 2023
dead10ck
dead10ck approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

Ahh, that makes sense, thanks! This looks good to me.

helix-loader/build.rs
.ok()
.filter(|output| output.status.success())
.and_then(|x| String::from_utf8(x.stdout).ok())
else{ return; };
Copy link
Member

Choose a reason for hiding this comment

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

Does cargo fmt work on build scripts?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but rustfmt currently ignores let .. else (leaves it unchanged) altough that will hopefully be fixed in the not-so-distant future

@the-mikedavis the-mikedavis merged commit 896404c into helix-editor:master Apr 14, 2023
@pascalkuthe pascalkuthe deleted the update-git-rev branch April 14, 2023 15:00
Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023
@pascalkuthe @Triton171
emit cargo metadata duiring build scripts to avoid outdated buildscri…
bf3f6a1 
…pt outputs (helix-editor#6743)

* rebuild on revision change
* rerun grammar build if grammars change
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@pascalkuthe @wes-adams
emit cargo metadata duiring build scripts to avoid outdated buildscri…
fdefb16 
…pt outputs (helix-editor#6743)

* rebuild on revision change
* rerun grammar build if grammars change
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@pascalkuthe
emit cargo metadata duiring build scripts to avoid outdated buildscri…
524852b 
…pt outputs (helix-editor#6743)

* rebuild on revision change
* rerun grammar build if grammars change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dead10ck dead10ck dead10ck approved these changes

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pascalkuthe @dead10ck @the-mikedavis