Skip to content

GitUp. libgit2 updated. v1.3.0#772

Closed
lolgear wants to merge 21 commits into
git-up:masterfrom
lolgear:gitup_libgit_updated
Closed

GitUp. libgit2 updated. v1.3.0#772
lolgear wants to merge 21 commits into
git-up:masterfrom
lolgear:gitup_libgit_updated

Conversation

@lolgear
Copy link
Copy Markdown
Contributor

@lolgear lolgear commented Oct 12, 2021

This PR updates libgit2 to version v1.3.0.

Related PR in gitup libgit2 fork.
git-up/libgit2#7

You need to run sh ./update_xcode.sh script to generate necessary file features.h

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear I can't for the life of me figure out how to get the latest version of what I merged into the gitup_libgit_updated branch.

I've run the following and get the following result:

>>> swift package update
Everything is already up-to-date
'SwiftPackage' /Users/lucasderraugh/Developer/GitUp/GitUpKit/Third-Party: error: invalid custom path './libgit2/deps/http-parser' for target 'http-client'

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Oct 26, 2021

@lucasderraugh
Sure, this PR doesn't point to correct libgit2 commit.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Oct 26, 2021

@lucasderraugh Please, check latest changes.
I added submodule commit.

@NikKovIos
Copy link
Copy Markdown
Contributor

Merge please.

@NikKovIos
Copy link
Copy Markdown
Contributor

@lolgear fix conflicts please.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

I fixed them up. I can't ask @lolgear to fix changes all the time 😓.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear When I run ./update-xcode.sh I get the following:

-- The C compiler identification is AppleClang 13.0.0.13000029
CMake Error at CMakeLists.txt:14 (PROJECT):
  No CMAKE_C_COMPILER could be found.



-- Configuring incomplete, errors occurred!
See also "/Users/lucasderraugh/Developer/GitUp/GitUpKit/Third-Party/libgit2/xcode/CMakeFiles/CMakeOutput.log".
The file /Users/lucasderraugh/Developer/GitUp/GitUpKit/Third-Party/libgit2/xcode/libgit2.xcodeproj does not exist.

Any ideas why the first part doesn't work? Also, the path you're trying to open at the end of that script seems incorrect, but not relevant to the errors I'm receiving.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Feb 24, 2022

Yes, it should use dirname $0.
Maybe a problem with invocation directory.
Do you try to run this script from libgit2 directory?

sh ./update_xcode.sh

@lucasderraugh
Copy link
Copy Markdown
Collaborator

lucasderraugh commented Feb 24, 2022

@lolgear Alright, so I figured out what I was doing wrong. It now runs, but I'm hitting this assertion in CGRepository:120:

assert(git_libgit2_features() & GIT_FEATURE_SSH);

Any ideas how that could be resolved?

Looks like you're trying to override it to be 1 in the Package.swift file but that doesn't seem to be working.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Feb 24, 2022

Hm, I guess it should be solved via Package.swift. This file "features.h" should always have _SSH macros. And update_xcode.sh should copy correct features.h file.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

Ya, unfortunately it's not being set correctly. My features.h file looks like this:

/* #undef GIT_SSH */
/* #undef GIT_SSH_MEMORY_CREDENTIALS */

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Feb 26, 2022

@lucasderraugh
Yeah, I checked it.
Hm, it would be nice to use SPM new features ( after Xcode 13.3 release ).
For now we can add extra step which fixes features.h file.

Could you check it?
git-up/libgit2#8

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear Is there a feature in Xcode 13.3 that makes the existing code work?

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented May 25, 2022

@lucasderraugh

Invalid exclude is an entry in exclude list in Third-Party/Package.swift.
But second one, hm, it should work without futimens.
What version of Xcode/macOS do you use?

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented May 25, 2022

@lucasderraugh
I updated exclude list and commit of submodule ( oh, submodules ).
Can you rerun update-xcode.sh again?

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 11, 2022

@lucasderraugh
Can we merge this PR?
It is a blocker for further improvements ( as I described above ).

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear Sorry I was out sick for the past week, getting caught up on normal work but I'll visit this on the weekend.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

lucasderraugh commented Jun 23, 2022

@lolgear Looks good from what I can tell. However, I want to fix our test suite before I merge this in to make sure I'm not missing anything. For some reason the xctest bundle isn't working on my end so I'll have to debug that a bit more tonight.

On top of that I'll need to bump to 10.13 before then because of the endless futimens warnings.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 24, 2022

@lucasderraugh
Hope you are doing well.
I'm curious in futimens warnings appearance, because it should be fixed.
I asked libgit2 maintainers to include this fix before 1.3.0 release.
Could you tell your current setup?
macOS, Xcode, commits ( in GitUp / forked libgit2 ), contents of update_xcode.sh.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 24, 2022

I checked this PR:

// update-xcode.sh
MACOSX_DEPLOYMENT_TARGET=10.10 cmake -S "$SCRIPT_DIRECTORY" -B "$SCRIPT_DIRECTORY/xcode" -G "Xcode"

and it's output is:

// sh update-xcode.sh
-- Looking for futimens
-- Looking for futimens - not found

and no warning at all...

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear Using Xcode 13.4, macOS 12.4. When I run it I get:

-- Looking for futimens
-- Looking for futimens - found

I'm on commit f2300f3 for this repo and b80dc34dd3b4abe59b5543efd849102ac75b7032 for libgit2 repo (our fork)

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 25, 2022

And what version of cmake?
I have cmake 3.22.3.
Do you run update-xcode.sh script without setting environment variables. MACOSX_DEPLOYMENT_TARGET should be 10.10.
It shouldn't find futimens at all, because deployment target is too low and futimens doesn't exist prior 10.13.

Could you check that

/// GitUpKit/Third-Party/libgit2/src/CMakeLists.txt 
/// contains
IF (HAVE_FUTIMENS)
  SET(GIT_USE_FUTIMENS 1)
ENDIF ()

and file GitUpKit/Third-Party/libgit2/cmake/Findfutimens.txt exists.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

lucasderraugh commented Jun 25, 2022

I have 3.23.1 and CMakeLists have

IF (HAVE_FUTIMENS)
	SET(GIT_USE_FUTIMENS 1)
ENDIF ()

I also have Findfutimens.cmake if that's what you meant.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 25, 2022

I upgraded cmake to 3.23.2.

And yes

-- Looking for futimens
-- Looking for futimens - found

Yes, it is a bug in ( I don't know, ah ) cmake? ( again? )
Because cmake doesn't respect MACOSX_DEPLOYMENT_TARGET environment variable.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jun 25, 2022

However, Apple will raise deployment targets to 10.13 in Xcode 14, so, it seems impossible to build products for 10.12 and less.

Not sure if it is necessary to fix this cmake issue.

https://developer.apple.com/documentation/Xcode-Release-Notes/xcode-14-release-notes

Building for deployment to OS releases older than macOS 10.13, iOS 11, tvOS 11, and watchOS 4 is no longer supported. (92834476)

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@lolgear I have most of the changes needed for the 10.12 bump. I'll try to get the build to 10.13 and then try this out again.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jul 2, 2022

@lucasderraugh
Can we merge this PR?

@lucasderraugh
Copy link
Copy Markdown
Collaborator

Working on testing it out. It will be the next PR that goes into GitUp, I promise.

Comment thread GitUpKit/Third-Party/Package.swift
Copy link
Copy Markdown
Collaborator

@lucasderraugh lucasderraugh left a comment

Choose a reason for hiding this comment

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

Requesting above changes.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jul 2, 2022

@lucasderraugh I added supported platforms.
I guess that you can raise also deployment target version in update-xcode.sh script (in libgit2 repository), but it doesn't work now so, not a big trouble
I mean that cmake doesn't recognize this variable (MACOSX_DEPLOYMENT_TARGET) or it doesn't use it.

Copy link
Copy Markdown
Collaborator

@lucasderraugh lucasderraugh left a comment

Choose a reason for hiding this comment

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

Unfortunately, the GitUpKit (macOS) tests don't compile, and even after fixing that, they don't pass. I'll see if I can figure out what's causing the failures, but we can't merge this in until the macOS tests are passing.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Jul 6, 2022

It is a hard decision, but... I suggest to test it in pre-release. ( and fix it during modernization )
Actually, this project requires a lot of time for modernization. ( remove MRC, move to SPM, cleanup tests/headers etc. )

GitUpKit won't pass tests, because so many changes happened in past years and libgit2 shows a lot of errors in tests.

We can use different branch ( dev? ) for pre-releases, so, only critical fixes will be in master branch.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

The tests are there to prevent this exact issue. We need to validate that we haven't broken existing functionality or if we are understand why. I've reviewed some already but it will take some more time on my end.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

An update on my testing, the failing tests have a number of valid issues. The current one that needs resolution is clone from URL which just fails (in tests and UI). Need some more time to resolve what has changed to cause this.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

Closing as I had other changes I needed to make before merging in and can't push to this PR. Thanks again for the help.

@lolgear
Copy link
Copy Markdown
Contributor Author

lolgear commented Aug 15, 2022

@lucasderraugh
I suggested to use a different branch ( dev? ) for this PR.
Instead you decided to fix tests.
Also I believe that this PR (#852) is the most valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants