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

Add git-lfs to path #1503

Closed
1 task done
pascalberger opened this issue Feb 18, 2018 · 27 comments
Closed
1 task done

Add git-lfs to path #1503

pascalberger opened this issue Feb 18, 2018 · 27 comments
Labels
Milestone

Comments

@pascalberger
Copy link

  • I was not able to find an open or closed issue matching what I'm seeing

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
$ git --version --build-options

git version 2.16.1.windows.4
cpu: x86_64
built from commit: ef6d451bbfef86a529ebf12620289e0f15a93d8e
sizeof-long: 4
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.16299.248]
  • What options did you set as part of the installation? Or did you choose the
    defaults?
# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: VIM
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

None

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

  • What commands did you run to trigger this issue? If you can provide a
    Minimal, Complete, and Verifiable example
    this will help us understand the issue.

Installer

  • What did you expect to occur after running these commands?

It would be nice if installing Git for Windows with git-lfs is sufficient to have LFS support in VIsual Studio.

  • What actually happened instead?

Git for Windows installs git-lfs to c:\Program Files\Git\mingw64\bin\git-lfs.exe, without git-lfs.exe being available on the path. While this works for standalone Git installation, this doesn't work for Visual Studio which has its own Git installation and cannot find git-lfs. Therefore currently it is required to still install git-lfs using the standalone installer (or put git-lfs in the path otherwise)

  • If the problem was occurring with a specific repository, can you provide the
    URL to that repository to help us with testing?

N/A

@shiftkey
Copy link

While this works for standalone Git installation, this doesn't work for Visual Studio which has its own Git installation and cannot find git-lfs.

This behaviour was changed a while ago, git-for-windows/build-extra#141 is the context, and I'll point out this comment:

This prevents an issue where the user may have an older version of LFS
installed by itself and 'git-lfs' finds that version while 'git lfs' finds
the bundled version.

I'll also let @dscho chime in and see if he knows anyone on the VS team who look after the Git integration to see if they can provide any additional help.

@dscho
Copy link
Member

dscho commented Feb 19, 2018

@shiftkey thank you for explaining the reasoning. That's precisely what my thinking was, too.

As to Visual Studio supporting Git LFS: If I remember correctly, the official Git LFS installer from the Git LFS project installs Git LFS into the PATH, and that should address your concerns.

I deem this ticket independent of Git for Windows itself.

@dscho dscho added the question label Feb 19, 2018
@pascalberger
Copy link
Author

This prevents an issue where the user may have an older version of LFS installed by itself and 'git-lfs' finds that version while 'git lfs' finds the bundled version.

But this is the current behavior. If I have Git for Windows with Git LFS installed and also the standalone Git LFS installer, git lfs will use LFS version shipped with Git for Windows, while git-lfs will use the version from the standalone installer from cmd.exe. It will use the version from Git for Windows for both commands in a Git bash shell though, as mingw32\bin is on the path there. So behavior is currently inconsistent between different shells.

Using the standalone Git LFS installer puts git-lfs on the PATH, and is what we're using currently for getting LFS support in Visual Studio. But it would be nice if we can use the Git LFS version shipped with Git for Windows.

@dscho
Copy link
Member

dscho commented Feb 19, 2018

@pascalberger I understand your concerns. In your scenario, I would highly recommend not using Git for Windows' embedded Git LFS, but only the Git LFS installed by the official Git LFS installer.

@dscho
Copy link
Member

dscho commented Feb 20, 2018

@pascalberger unless you have an idea how we can make your desired feature work: we cannot add mingw64\bin to the PATH, as it has too much stuff in it that we do not want to expose to the user. For example, this addition would override find and sort, breaking all kinds of Windows scripts...

I'll keep this ticket open for a couple of days in the hope that you can come up with a solution other than what I suggested.

@pascalberger
Copy link
Author

@dscho Would it be possible to add a symlink for git-lfs.exe to the bin path which links to the exe in the mingw64\bin path? Or how is it done for git.exe?

@dscho
Copy link
Member

dscho commented Feb 20, 2018

@pascalberger for git.exe, there is a wrapper in cmd\git.exe which simply calls mingw64\bin\git.exe after setting up a couple of environment variables. We could of course do the same for git-lfs.exe, for which we would most likely have to add some code here (imitating the git.exe case to a certain extent).

And then we would need to hard-link/copy the wrapper to git-lfs.exe by calling CopyBuiltin(AppDir+'\cmd\git-lfs.exe'); probably at the same location where the builtins are hard-linked/copied, but guarded by an if IsComponentSelected('gitlfs') then ....

We would then also have to take care of deleting that file at uninstall time, most likely it will suffice to add it to the [UninstallDelete] section.

@pascalberger how about giving it a try?

@stffabi
Copy link

stffabi commented Feb 21, 2018

@dscho I've taken over the task of @pascalberger and tried to adjust the wrapper and the installer. I've created two pullrequests git-for-windows/MINGW-packages#22 and git-for-windows/build-extra#173

I was able to compile the wrapper and test it. But for the installer I'm not sure, if I'll succeed. I'll do my best and will let you know 😄...

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Thank you! @stffabi!

@dscho dscho closed this as completed Feb 21, 2018
@dscho dscho added this to the v2.16.2(2) milestone Feb 21, 2018
dscho added a commit to git-for-windows/build-extra that referenced this issue Feb 21, 2018
When choosing to "Use Git from the Windows Command Prompt" (i.e. add
only the minimal set of Git executables to the `PATH`), and when
choosing the Git LFS component, Git LFS [is now included in that
minimal set](git-for-windows/git#1503).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@stffabi
Copy link

stffabi commented Feb 21, 2018

@dscho thanks so much for merging the changes. I was just able to compile the new installer and test it. Unfortunately it seems like the CopyBuiltin currently doesn't work as we both expected.

On line L1590 for newer windows versions the target of the hardlink is git.exe. Therefore this won't work for the git-lfs case. Sorry for that, but I've just seen that the second you've merged...

Is this the intended behaviour or should this case also be hardlinked to git-wrapper.exe? Or was here the intention to only use the git-wrapper.exe in the fallback case where normal copies of the file will be created and to minimize the used disk space there?

@dscho
Copy link
Member

dscho commented Feb 21, 2018

On line L1590 for newer windows versions as target for the hardlink git.exe will be used. Therefore this won't work for the git-lfs case

But it is bin\git.exe that is used. That should be a copy of the Git wrapper itself...

@dscho
Copy link
Member

dscho commented Feb 21, 2018

(Note the difference between bin\git.exe, which is installed only for legacy purposes, and mingw64\bin\git.exe, which is the real git.exe.)

@stffabi
Copy link

stffabi commented Feb 21, 2018

Okey, but then installer seems to use the wrong destination.

Line 1590: LinkCreated:=CreateHardLink(FileName,AppDir+'\{#MINGW_BITNESS}\bin\git.exe',0);

The resulted hardlink would map to mingw64\bin\git.exe and therefore the real git.exe.

@stffabi
Copy link

stffabi commented Feb 21, 2018

So we should change the line to: LinkCreated:=CreateHardLink(FileName,AppDir+'\bin\git.exe',0);

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Oh, you're right! Darn, my brain is rotten today. I'll fix it up.

@sentiment-bot
Copy link

sentiment-bot bot commented Feb 21, 2018

Please be sure to review the code of conduct and be respectful of other users. cc/ @git-for-windows/trusted-git-for-windows-developers

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Thanks @sentiment-bot! I am sorry 😄

@stffabi
Copy link

stffabi commented Feb 21, 2018

@dscho I may create another PR for this if you would like, I'm just testing the change :)

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Oh, I have the change, too...

@dscho
Copy link
Member

dscho commented Feb 21, 2018

@stffabi
Copy link

stffabi commented Feb 21, 2018

Great, thanks then I'll let you merge yours :)...

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Could you give this a quick glance-over?

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Now it is even a PR: git-for-windows/build-extra#174

@dscho
Copy link
Member

dscho commented Feb 21, 2018

Oh, and if you could test it, that would be awesome and give me the time to look at two other tickets... 😊

@stffabi
Copy link

stffabi commented Feb 21, 2018

Sure, I'll let you know the results of my test.

@dscho
Copy link
Member

dscho commented Feb 21, 2018

@stffabi thank you so much!

@stffabi
Copy link

stffabi commented Feb 21, 2018

@dscho you're welcome. I've left some comments directly on the PR.

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

No branches or pull requests

4 participants