Skip to content

cli: lazily emit cargo:rerun-if-changed to avoid spurious rebuilds #7273

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

cli: lazily emit cargo:rerun-if-changed to avoid spurious rebuilds #7273

wants to merge 6 commits into from
+42 −29

Conversation

ipetkov
Copy link
Contributor

@ipetkov ipetkov commented Aug 16, 2025

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added/updated tests to cover my changes

@ipetkov ipetkov requested a review from a team as a code owner August 16, 2025 19:05
@ipetkov
Copy link
Contributor Author

ipetkov commented Aug 16, 2025

Hmm test suite passed for me on the initial run, but now that I have this merged into another branch I see the test_global_opts::test_version failing consistently 🤔

thread 'test_global_opts::test_version' (3658728) panicked at cli/tests/test_global_opts.rs:61:5:
    `jj version` output: "jj 0.32.0-dirty\n".
    Sanitized: "jj ?.??.?-?irty\n"
    Expected one of: ["jj ?.??.?\n", "jj ?.??.?-????????????????????????????????????????\n"]
    stack backtrace:
       0: __rustc::rust_begin_unwind
       1: core::panicking::panic_fmt
       2: runner::test_global_opts::test_version
       3: runner::test_global_opts::test_version::{{closure}}
       4: core::ops::function::FnOnce::call_once
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@ipetkov
Copy link
Contributor Author

ipetkov commented Aug 16, 2025

Maybe another option to hardcoding NIX_JJ_GIT_HASH = self.rev or "dirty"; would be to change it to NIX_JJ_GIT_HASH = self.rev or ""; in the devshell, and then tweak the build script to not ignore when $NIX_JJ_GIT_HASH is set but empty? Not fully sure the implications of doing that (would this break version output for Nix builds?) so I could use a pointer on whether there's a preferable way to resolve this or just live with the spurious rebuilds and close this PR

yuja
yuja reviewed Aug 17, 2025
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

cc @thoughtpolice as I have no expertise on the nix part.

@ipetkov ipetkov force-pushed the ivan/push-tvppvspulrvn branch 2 times, most recently from 7d7a4c9 to fd5daa0 Compare August 20, 2025 19:23
@ipetkov ipetkov requested review from yuja and thoughtpolice August 20, 2025 19:32
yuja
yuja reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

The changes in build.rs look good. I can't review the nix change, so I'll leave it to someone else.

@ipetkov ipetkov force-pushed the ivan/push-tvppvspulrvn branch from fd5daa0 to 396fd95 Compare August 22, 2025 22:03
@martinvonz
Copy link
Member

The changes in build.rs look good. I can't review the nix change, so I'll leave it to someone else.

Maybe @thoughtpolice or @emilazy can help?

Minion3665
Minion3665 reviewed Aug 23, 2025
View reviewed changes
flake.nix
@@ -201,7 +201,10 @@
pkgs.mkShell {
name = "jujutsu";
packages = packages ++ nativeBuildInputs ++ buildInputs ++ nativeCheckInputs;
inherit env shellHook;
env = env // {
NIX_JJ_GIT_HASH = self.rev or "dirty";
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird to me - but only because for the package there is

          env =
            env
            // {
              RUSTFLAGS = pkgs.lib.optionalString pkgs.stdenv.isLinux "-C link-arg=-fuse-ld=mold";
              NIX_JJ_GIT_HASH = self.rev or "";
            };

on line 112

Is there any reason you set the default to "dirty" here rather than remaining consistent with the existing code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind - realized you're filtering to make sure it's not empty in your get_git_hash_from_nix function 🤦🏼‍♀️ - sorry for the noise

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case the nix side of things looks good to me, that was my only complaint :)

though I haven't tested it to see

ipetkov added 5 commits August 31, 2025 18:20
@ipetkov
cli: split each git hash source into its own build script function
baa645d
@ipetkov
cli: lazily emit cargo:rerun-if-changed to avoid spurious rebuilds
bbdd116 
The build script has a hierarchy of where the current git hash is
detected from: check $NIX_JJ_GIT_HASH, if that fails, ask jj, if that
fails, ask git.

If the build script is satisfied from a query higher up the priority
chain, there's no reason to instruct cargo to rereun and rebuild (and
relink) everything: for example if $NIX_JJ_GIT_HASH is set, there's no
need to check heads from either jj or git.

Although the build script takes effort to specify `jj log
--ignore-working-copy`, it would previously still get invalidated if jj
performed a snapshot (even if $NIX_JJ_GIT_HASH was set). This can be
fairly annoying when iterating on the test suite where each commit would
rebuild the CLI and by extension all of its tests.
@ipetkov
nix: set NIX_JJ_GIT_HASH to "dirty" if no revision is found
7bece62 
This effectively avoids churning the CLI build script every time a jj
commit (or snapshot) is updated which can speed things up during
development (especially when iterating on the test suite and not having
to wait for the CLI and its tests to rebuild and relink)
@ipetkov
cli: rewrite get_git_hash_from_git in a functional style
7b09cf5
@ipetkov
cli: rewrite get_git_hash_from_jj in a functional style
6052d9e
@ipetkov ipetkov force-pushed the ivan/push-tvppvspulrvn branch from 396fd95 to 6052d9e Compare September 1, 2025 01:25
@ipetkov
Copy link
Contributor Author

ipetkov commented Sep 1, 2025

Hmm test suite passed for me on the initial run, but now that I have this merged into another branch I see the test_global_opts::test_version failing consistently 🤔

Ended up adding an allowance for version strings ending with a -dirty suffix for debug builds, please let me know if this is not something we want to have!

ilyagr
ilyagr reviewed Sep 1, 2025
View reviewed changes
@ipetkov ipetkov force-pushed the ivan/push-tvppvspulrvn branch from 67310d1 to 10693ae Compare September 2, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja left review comments

@ilyagr ilyagr ilyagr left review comments

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

+1 more reviewer

@Minion3665 Minion3665 Minion3665 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ipetkov @martinvonz @yuja @ilyagr @Minion3665