Skip to content

git: make git2 support optional #5956

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
Merged
merged 4 commits into from
Mar 16, 2025
Merged

git: make git2 support optional #5956

merged 4 commits into from
Mar 16, 2025

Conversation

emilazy
Copy link
Contributor

@emilazy emilazy commented Mar 9, 2025

This helps us prepare for removing the functionality down the line and makes things easier for people building or packaging their own Jujutsu.

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 tests to cover my changes

@demize
Copy link
Contributor

demize commented Mar 9, 2025

I'm not 100% sure on why, but in my testing this out as an ebuild, I'm running into this while building with git2:

>>> Install dev-vcs/jj-9999 into /var/tmp/portage/dev-vcs/jj-9999/image
 * /usr/lib/rust/9999/bin/cargo install --root /var/tmp/portage/dev-vcs/jj-9999/image/usr --path cli
  Installing jj-cli v0.27.0 (/var/tmp/portage/dev-vcs/jj-9999/work/jj-emilazy-push-xvnqslxuupmv/cli)
error: failed to compile `jj-cli v0.27.0 (/var/tmp/portage/dev-vcs/jj-9999/work/jj-emilazy-push-xvnqslxuupmv/cli)`, intermediate artifacts can be found at `/var/tmp/portage/dev-vcs/jj-9999/work/jj-emilazy-push-xvnqslxuupmv/target`.
To reuse those artifacts with a future compilation, set the environment variable `CARGO_TARGET_DIR` to that path.

My wild speculation as to what's causing this is the addition of "testutils/git2" to the features enabled by "git2". It builds fine (though I think I'm running into issues with the tests), but then fails to install.

Builds and installs fine without git2, so that's nice, though I'm also running into issues with the tests there (the same one that triggered the CI failure, and also some others I think may be unrelated regressions since 0.27 that I need to look into more).

@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch 3 times, most recently from a80f5e0 to 28e2c10 Compare March 10, 2025 00:58
@emilazy
Copy link
Contributor Author

emilazy commented Mar 10, 2025

Pushed a horrible but apparently working fix for @demize’s issue.

bsdinis
bsdinis reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor

@bsdinis bsdinis left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! looks generally good, but I think we might want to warn users that have a version of jj compiled without git2 that they are trying to use the git2 code at config parsing. This is also where the future deprecation warning will come from and seems simpler to me

@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from 28e2c10 to 8fe3023 Compare March 10, 2025 17:11
@emilazy emilazy force-pushed the emilazy/push-wvtxvpwzzmko branch 2 times, most recently from c7badf6 to 5689787 Compare March 10, 2025 18:01
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch 2 times, most recently from 2b1fba6 to 9ac37ed Compare March 10, 2025 21:21
@emilazy emilazy force-pushed the emilazy/push-wvtxvpwzzmko branch 2 times, most recently from 23b5ff1 to 2c577be Compare March 10, 2025 22:06
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from 9ac37ed to 68fcd4f Compare March 10, 2025 22:06
Base automatically changed from emilazy/push-wvtxvpwzzmko to main March 11, 2025 03:25
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from 68fcd4f to 1d53645 Compare March 11, 2025 03:31
@emilazy
Copy link
Contributor Author

emilazy commented Mar 11, 2025

Rebased to make GitHub less confused about how many commits there are here. (And also to get a measurement for how much longer the extra job will make CI.)

@emilazy
Copy link
Contributor Author

emilazy commented Mar 11, 2025

Looks like this shouldn’t lengthen CI times appreciably. Hopefully we can cut down on the redundant push/pull_request checks anyway.

This should be ready for review/merge now.

@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from 1d53645 to fdd770b Compare March 11, 2025 04:14
yuja
yuja approved these changes Mar 12, 2025
View reviewed changes
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from fdd770b to 34123e3 Compare March 15, 2025 00:23
@emilazy
git: inline open_git_repo
94505f3 
The fetch and push code are the only remaining users of this, so
let’s not encourage adding any more.
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch 2 times, most recently from 4f96e2c to 48a4300 Compare March 15, 2025 22:18
emilazy added 3 commits March 16, 2025 01:07
@emilazy
git: make git2 support optional
e5f7736 
This helps us prepare for removing the functionality down the line and
makes things easier for people building or packaging their own Jujutsu.
@emilazy
config: warn on unsupported git.subprocess = false
69d7fde
@emilazy
github: add variant without git2 to CI
8104b65 
Just using the fastest platform should be fine for this. Hopefully it
shouldn’t slow down CI too much since it’s an independent build
job and only temporary, though a potential alternative would be to
just check the build instead. (It wouldn’t catch build regressions
in the test code, though.)
@emilazy emilazy force-pushed the emilazy/push-xvnqslxuupmv branch from 48a4300 to 8104b65 Compare March 16, 2025 04:41
@emilazy emilazy added this pull request to the merge queue Mar 16, 2025
Merged via the queue into main with commit 8bb6b90 Mar 16, 2025
26 checks passed
@emilazy emilazy deleted the emilazy/push-xvnqslxuupmv branch March 16, 2025 06:25
This was referenced Mar 10, 2025
Closed
Merged
@yuja yuja mentioned this pull request Apr 3, 2025
Closed
4 tasks
@emilazy emilazy mentioned this pull request Apr 3, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja approved these changes

@demize demize demize left review comments

@bsdinis bsdinis bsdinis left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@emilazy @demize @yuja @bsdinis