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.

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

fix: search at first system header for git2 then vendor's one #950

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

majioa
Copy link

@majioa majioa commented Mar 2, 2023

  • ! search at first system header for git2 then vendor's one. please refer to patch

! search at first system header for git2 then vendor's one
@carlosmn
Copy link
Member

Is there a comment on what this is doing or why? The patch referenced is just the diff.

@majioa
Copy link
Author

majioa commented Apr 17, 2023

In case the package libgit2-devel/dev is installed, the headers is picking up from them.

@carlosmn
Copy link
Member

carlosmn commented Jun 8, 2023

That seems to be the opposite of what path is trying to do. But still, that doesn't tell me what this is trying to fix by searching in both when the options are for rugged to try to pick up either a system-installed libgit2 with the right version, OR use the vendored version.

If you ask it to use the vendred version, then it should use everything from the vendored libgit2. If you ask it to use the system version, then everything should come from the system version.

The logic you're modyfing is for the `--use system libraries" logic. If there is a libgit2 installed then it should only look at it. Looking at it now, it does look like it could buggy and it might be at the vendored header instead of the system header that was found. But then that's what needs to get fixed.

@carlosmn
Copy link
Member

carlosmn commented Jun 8, 2023

I was misreading the code. The code you are modifying does want to look at the vendored version there. What it's doing is extracting the version of libgit2 that's compatible with the version of rugged that's getting compiled. Looking at the system header instead would invalidate any verification, as it would be verifying that the installed version matches the installed version.

@majioa
Copy link
Author

majioa commented Jun 9, 2023

may be there is a sense to add "enable system library" explicitly like in nokogiri gem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants