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 support for different LFS url to vcpkg_from_git #28693

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

DDoS
Copy link
Contributor

@DDoS DDoS commented Jan 3, 2023

Add an optional argument to the LFS keyword in vcpkg_from_git, so a different URL can be used. Update the documentation and tests.

Fixes #28593

github-actions[bot]
github-actions bot previously approved these changes Jan 3, 2023
@JonLiu1993 JonLiu1993 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 Jan 4, 2023
@DDoS
Copy link
Contributor Author

DDoS commented Jan 4, 2023

The tests are failing on Windows because the agent image has an old version of Git LFS: 2.13.3 which is almost two years out of date. The test script needs the --force flag on prune, which is a feature that came in the next release (3.0.0). I also noticed that the macOS and Linux images don't have LFS installed.

I'm not sure how to fix this other than updating the VM images, which I don't know how to do.

@JonLiu1993
Copy link
Member

The tests are failing on Windows because the agent image has an old version of Git LFS: 2.13.3 which is almost two years out of date. The test script needs the --force flag on prune, which is a feature that came in the next release (3.0.0). I also noticed that the macOS and Linux images don't have LFS installed.

I'm not sure how to fix this other than updating the VM images, which I don't know how to do.

@vicroms, could you please take a look?

@vicroms vicroms added depends:vm-update PR contains changes to the VM provisioning scripts requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jan 9, 2023
@vicroms
Copy link
Member

vicroms commented Jan 9, 2023

@BillyONeal would it be possible to update Git LFS on our VMs?

@BillyONeal
Copy link
Member

@BillyONeal would it be possible to update Git LFS on our VMs?

We are deploying the copy that comes with Visual Studio. If the user needs a copy more recent than that I have serious concerns about being able to accept that in a port.

@DDoS
Copy link
Contributor Author

DDoS commented Jan 9, 2023

@BillyONeal This is just for testing. The scripts don't need anything more recent normally, but I can't implement a proper test without some newer LFS features. I could just disable that part of the test if the LFS version is too old, but then it's not automatically tested...

@BillyONeal
Copy link
Member

@BillyONeal This is just for testing. The scripts don't need anything more recent normally, but I can't implement a proper test without some newer LFS features. I could just disable that part of the test if the LFS version is too old, but then it's not automatically tested...

We don't have a means of restricting such a machine-wide configuration change to your test.

@DDoS
Copy link
Contributor Author

DDoS commented Jan 10, 2023

@BillyONeal I'm surprised that the vcpkg VM images are even using a two year old Git LFS version... The default Azure hosted agent images are up-to-date.
Does anything else in vcpkg even depends on LFS? I was under the impression that there was no support for it until it was added to vcpk_from_git.
But anyway if updating LFS is not an option, I can disable the test for older versions, or just abandon this PR.

@BillyONeal
Copy link
Member

I'm surprised that the vcpkg VM images are even using a two year old Git LFS version...

That's what comes with Visual Studio 2022.

The default Azure hosted agent images are up-to-date.

The default Azure hosted images contain everything under the sun because they are trying to maximize the likelihood of your build working. vcpkg's lab contains only what a developer is likely to have installed for themselves because we don't want ports silently depending on one of those external dependencies like one might find on the default Azure hosted images.

Does anything else in vcpkg even depends on LFS?

Intentionally? Probably not. But we invoke arbitrary user build scripts which could.

But anyway if updating LFS is not an option, I can disable the test for older versions, or just abandon this PR.

Or we could just wait until a version which does what you need has enough market penetration that we can confidently assume its presence.

@DDoS
Copy link
Contributor Author

DDoS commented Jan 10, 2023

@BillyONeal How does vcpkg even handle testing ports that require specific tooling if it can't be installed? I know that vcpkg acquires some programs automatically, but looking at the code it just asks you to install it yourself in many cases.

Maybe the oldest viable LFS version (3.0.0 from March 2021) can be installed instead of the latest one? But if you insist on waiting for LFS to be updated externally, then feel free to close this PR. I don't personally need this change, I just did it because someone asked for it and I thought it would be a quick and easy. I don't want to spend that much more time working on it.

@BillyONeal
Copy link
Member

How does vcpkg even handle testing ports that require specific tooling if it can't be installed?

It depends. But usually these days we probably wouldn't accept adding stuff like that to the curated registry. Ports are on the hook for being broadly installable with a "typical developer workstation". Seeing that we have something only for it to explode in your face is a bad experience.

The one big notable exception I'm aware of right now is CUDA, but pretty much every port that wants CUDA has that controlled by an optional feature. (And there, it isn't that we wouldn't solve the problem of getting what's necessary, it's that we are legally unable to under NVIDIA's license terms)

But if you insist on waiting for LFS to be updated externally, then feel free to close this PR.

I will confirm with other maintainers before going there. (Should know by ~Thursday)

@BillyONeal
Copy link
Member

@vicroms @dan-shaw @ras0219-msft @AugP @JavierMatosD and I discussed this today and came up with the following outcomes:

  1. Because this feature doesn't break people who don't use it, we are happy to accept the new feature.
  2. We do not want to modify our VMs to install unlikely-to-be-what-users-use copies of git.
  3. We would accept the feature even if we can't test it right now; please include the test in a disabled form with a comment like "when (component) is available turn this test on"

Does that sound reasonable @DDoS ?

@DDoS
Copy link
Contributor Author

DDoS commented Jan 12, 2023

@BillyONeal Sounds good. I'm away for a week, so I'll make this change next Thursday or Friday.

@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 12, 2023
Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

As discussed, disable test until we are ready to enable it in our VMs.

@DDoS
Ping me when done to merge this PR! 😄

@BillyONeal BillyONeal removed the depends:vm-update PR contains changes to the VM provisioning scripts label Jan 14, 2023
@DDoS
Copy link
Contributor Author

DDoS commented Jan 20, 2023

@horenmar Can you test if these changes fix your issue (#28593)?

@vicroms I added a version check to the test. That way it should enable automatically when a suitable LFS version is available.

@vicroms vicroms added the info:reviewed Pull Request changes follow basic guidelines label Jan 20, 2023
@BillyONeal
Copy link
Member

I will merge this closer to the end of the day. (Just don't want to push a merge conflict with the world while still processing today's changes)

@BillyONeal BillyONeal merged commit 50805d6 into microsoft:master Jan 24, 2023
@BillyONeal
Copy link
Member

Thank you!

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 info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[portfile helpers] vcpkg_from_git should support LFS for/from arbitrary endpoint
4 participants