-
Notifications
You must be signed in to change notification settings - Fork 349
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
cargo: bump git2
to 0.19.0
#4080
Conversation
Hmm, I'm not very sure how to fix the Windows failure. (I don't have a Windows machine to debug this.) |
af40a64
to
5f81789
Compare
git2
to 0.19.0git2
to 0.19.0
This comment was marked as outdated.
This comment was marked as outdated.
Maybe we can apply |
@yuja: Looks like the problem is twofold:
So, we'll need to use |
I think this is likely a regression in libgit2, since we didn't touch the code on our side at all. |
@bnjmnt4n: Actually, you'll get errors in vanilla Git if you try to use UNC paths for a remote, too, though only when you try to actually use it:
So this is likely validation in |
If that's what FWIW, I think git config would support some form of backslash escapes (because |
|
I am also having trouble installing via homebrew. Uninstalling and reinstalling does not seem to work for me:
|
Oh yes, I spoke too hastily 😉 It installs, but doesn't run. Details
~
❯ jj --version
dyld[12839]: Library not loaded: /opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib
Referenced from: <E8960ED2-AAE3-34EB-9B6F-057F96828A15> /opt/homebrew/Cellar/jj/0.19.0/bin/jj
Reason: tried: '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file)
zsh: abort jj --version |
Here's the issue for Homebrew: Homebrew/homebrew-core#177686. The temporary fix would be to update the formula to point to |
@bnjmnt4n: While the test in CI has technically regressed, I'm not sure that this is a regression that's notable for users. Users who succeeded in cloning wouldn't have been able to use their local Git remotes anyway, which is behavior that I don't think this upgrade changes. Is that something that should block this upgrade, as opposed to having a separate issue filed? |
I can disable the test on Windows to get this merged in, if that's what's desired by other contributors. |
506c37c
to
9a6e5d4
Compare
For future reference, here's the last CI run that demonstrates the error if the test is not disabled on Windows: https://github.com/martinvonz/jj/actions/runs/10114947983/job/27974690965
|
For my "diffedit3", I was initially going to use https://docs.rs/path-slash/latest/path_slash/ to make tests pass on Windows, but then eventually settled on: fn to_slash_string_lossy(path: &Path) -> String {
path.to_string_lossy().replace('\\', "/")
} I only did this to make the test output consistent, and here we also need and the other uses of |
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'm not sure whether it's better to merge this in before the release next week or to wait until after, but I'm leaning towards the latter. What do people think?
I think it's fine to disable the test for a short time if we fix it later, but I'm slightly worried if there are other changes in behavior that the new libgit2
will cause.
This also affects packagers (though I think it's not a huge consideration; they'll upgrade eventually). From a glance, Homebrew has both versions of libgit2
packaged, but Nixpkgs doesn't yet have libgit2
1.8 packaged. I'm not sure whether Nixpkgs will go the same route as Homebrew and support two versions at once.
We could use this merging for Nixpkgs, if there aren’t any blockers left. |
There's some stuff with Windows test, but I think it might be a good idea to merge it now to have time to see what other stuff (if any) breaks. We have three options for the Windows test:
|
(Link should be #4184, for anyone wondering.) Disabling the test or your fix both sound like good options to me. But of course I’m biased; I want to keep the Jujutsu package in Nixpkgs building by including the patch :) #4234 suggests that it’s also an issue for Arch. (Of course, there’s a separate discussion to be had about vendoring libgit2, but that should probably be done under less time pressure.) |
Judging by #4184 (review), it sounds like @yuja would prefer this PR (with its TODO and disabled test) over merging #4184. That's fine by me. Yuya, is that right? WDYT? I might do some more work on #4188, but I don't want to promise any timeline (and anybody else is welcome to pick it up). |
Could always merge this now and do #4184 as another incremental step later if desired, right? (I’m assuming that the test failure is not actually substantive in such a way as to consider Windows broken after this PR.) |
Yes.
I'm... not actually sure. Judging by our tests, it's not too bad, but there also tend to be Windows-specific issues that aren't covered by our tests. One thought I'm having is "let's merge this now and see whether we get any complaints about something crucial not working on Windows anymore". There is a non-zero chance this PR would get reverted (Update: or perhaps reverted only on Windows), but it's not a high chance, and let's be optimistic :) |
It needs rebasing for conflicts, but if you want me to hit the merge button and take the blame for breaking Windows I’m up for it! |
9a6e5d4
to
098c016
Compare
I would prefer it if @yuja and/or @bnjmnt4n took a look. I think there's a reason this is not approved yet, I'm unsure whether we are all on the same page. (also, I'd like a "note to maintainers" as I said in another comment, but it's not crucial and could also go into a separate PR) @bnjmnt4n, let us know if you want one of us to take care of hitting the merge button when this is approved. By default, it's pretty much @bnjmnt4n 's sole right to hit the merge this AFAICT unless he tells us wants one of us to or we decide to make an exception (not that these rules have to by followed religiously, but these are the rules I usually go by). |
(Sorry if resolving the conflict was not the done thing!) |
Yes. Suppose it's an existing bug and these PRs don't fix the underlying problem, I would choose the minimal workaround. |
This includes a bump of `libgit2` to v1.8.1.
098c016
to
9a5d201
Compare
v0.21.0 has been tagged since this PR was merged, but I tried to build it on Arch Linux with linking the system libgit. Having hit an error I came back here because I thought I remembered this landing on main, but GitHub seems to thing it didn't make the release cut. I'm a bit puzzled since this merged to main and the tag was also released on main so ... anybody know what gives? |
We are building Jujutsu 0.21.0 with libgit2 1.8.1 in Nixpkgs without any problems, so I suspect something is wrong on the Arch end. 38f6ee8 shows up as being part of |
Thanks for the point of reference @emilazy, I'm looking into it. Edit: My linking issue actually turned out to be libssh2 related, not libgit2. Enabling the latter requires properly setting up the former, so enabling libgit2 linking was actually working but the libssh2 stuff it dragged in with it wasn't yet. I got that resolved. |
This includes a bump of
libgit2
to v1.8.1.Checklist
If applicable:
CHANGELOG.md