-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Add support for different LFS url to vcpkg_from_git #28693
Conversation
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 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? |
@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. |
@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. |
@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. |
That's what comes with Visual Studio 2022.
The default Azure hosted images contain everything under the sun because they are trying to maximize the likelihood of your build working.
Intentionally? Probably not. But we invoke arbitrary user build scripts which could.
Or we could just wait until a version which does what you need has enough market penetration that we can confidently assume its presence. |
@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. |
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)
I will confirm with other maintainers before going there. (Should know by ~Thursday) |
@vicroms @dan-shaw @ras0219-msft @AugP @JavierMatosD and I discussed this today and came up with the following outcomes:
Does that sound reasonable @DDoS ? |
@BillyONeal Sounds good. I'm away for a week, so I'll make this change next Thursday or Friday. |
There was a problem hiding this 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! 😄
8ec6a24
to
5f8c2ee
Compare
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) |
Thank you! |
Add an optional argument to the
LFS
keyword invcpkg_from_git
, so a different URL can be used. Update the documentation and tests.Fixes #28593