Skip to content

[vcpkg] Add support to pull from git-lfs#22158

Closed
ahojnnes wants to merge 1 commit intomicrosoft:masterfrom
ahojnnes:user/joschonb/vcpkg-from-git-lfs
Closed

[vcpkg] Add support to pull from git-lfs#22158
ahojnnes wants to merge 1 commit intomicrosoft:masterfrom
ahojnnes:user/joschonb/vcpkg-from-git-lfs

Conversation

@ahojnnes
Copy link
Contributor

Without this PR, checking out sources from a git repository with git-lfs enabled fails with the following error:

Use `git lfs logs last` to view the log.
error: external filter 'git-lfs filter-process' failed
fatal: <... git-lfs file path ...>: smudge filter lfs failed

@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Dec 22, 2021
@PhoebeHui
Copy link
Contributor

Related to #18570.

@PhoebeHui PhoebeHui changed the title Add support to pull from git-lfs [vcpkg] Add support to pull from git-lfs Dec 23, 2021
@ahojnnes
Copy link
Contributor Author

Yes, thanks @PhoebeHui. In the older PR, I mentioned that the issue was fixed but then I realized it actually isn’t, so I fixed it here.

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, we will ask other team member to take a look again。

@Cheney-W Cheney-W added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Dec 27, 2021
@BillyONeal
Copy link
Member

How common is it for the git bundled with most Linux distros to include git-lfs support?

Do you have any ports which fail without this change but don't fail afterwards? We shouldn't add random support here without a test case.

@ahojnnes
Copy link
Contributor Author

I have a custom portfile against a Microsoft internal repository with git-lfs enabled. Without this patch here, I am unable to checkout and build the code. I don’t know about a public repository but I could setup a mock repo to reproduce the issue. If you have an alternative suggestion to solve the issue, I’d be happy to hear. Otherwise I am also happy to add test coverage for this fix, if you can give me a pointer to where I should get started with that. Thanks.

@JackBoosY
Copy link
Contributor

Ping @BillyONeal for reply.

@BillyONeal
Copy link
Member

I have a custom portfile against a Microsoft internal repository with git-lfs enabled. Without this patch here, I am unable to checkout and build the code. I don’t know about a public repository but I could setup a mock repo to reproduce the issue. If you have an alternative suggestion to solve the issue, I’d be happy to hear. Otherwise I am also happy to add test coverage for this fix, if you can give me a pointer to where I should get started with that. Thanks.

Without a public user it's concerning to add since it adds a new dependency of vcpkg which ports may depend on by accident and thus blow up in users' face if lfs is not ~always installed on all the *nixes.

@strega-nil-ms
Copy link
Contributor

I'm not against this; I think it's entirely reasonable, and it doesn't break anybody. My questions are:

  • what happens with LFS data when the origin remote changes?
  • what happens when git-lfs isn't installed?
  • does LFS trigger trigger automatically, or is it something that's perhaps triggered by the build system
    • in other words, if you install a port that requires git-lfs, will it be obvious you need to install git-lfs or will it just fail in a weird way

@BillyONeal
Copy link
Member

I'm not against this; I think it's entirely reasonable, and it doesn't break anybody.

+1. I just realized my replies could have been understood to mean that I was opposed to this. Like @strega-nil I'm mostly ensuring we understand the consequences more than worried about the change itself.

@strega-nil-ms
Copy link
Contributor

unmarking requires:vcpkg-team-review until the author gets back to us.

@strega-nil-ms strega-nil-ms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 19, 2022
@ahojnnes
Copy link
Contributor Author

To reply to @strega-nil-ms questions above in order:

  1. Unless I missed something, each call to vcpkg_from_git will happily overwrite the existing origin URL with whatever URL was provided on the caller side. This assumes the git repository was already checked out using something like --editable? Not sure under which other scenarios, vcpkg would not check out a complete new clone of the repo?
  2. If git-lfs is not installed, there shouldn't be any noticeable difference.
  3. git-lfs will be used automatically, if the repo requires it.

@JackBoosY
Copy link
Contributor

Ping @strega-nil-ms for response.

@BillyONeal
Copy link
Member

  1. If git-lfs is not installed, there shouldn't be any noticeable difference.

Is it documented this way or have you actually tried it?

  1. git-lfs will be used automatically, if the repo requires it.

I don't think you answered the question. Note: "in other words, if you install a port that requires git-lfs, will it be obvious you need to install git-lfs or will it just fail in a weird way"

@JackBoosY
Copy link
Contributor

Ping for response.

@pavel-pokutnev
Copy link

I have the same issue. Just want to confirm that this patch is solving it on my side.

@JackBoosY
Copy link
Contributor

Convert this PR to draft since there is no response for a long time.

@JackBoosY JackBoosY marked this pull request as draft June 1, 2022 02:46
@JackBoosY
Copy link
Contributor

Please temporary close this PR if you don't have spare time to finish it.

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

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants