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

remove CMake and PowerShell versions from ABI info #263

Closed

Conversation

Be-ing
Copy link

@Be-ing Be-ing commented Nov 10, 2021

Neither of these actually affect the ABI. Including these in the ABI info is problematic because it requires using the exact same
versions of these tools on build servers as developers' local build environments to get binary caching to use cached packages
built on a server.

This is an alternative solution to microsoft/vcpkg#16615. Unlike #223 it avoids vcpkg needing to download exact versions of CMake and PowerShell if a newer version than vcpkg's minimum requirement is already available from the system.

I don't particularly care whether this or #223 is merged; I just want some solution to microsoft/vcpkg#16615.

Neither of these actually affect the ABI. Including these in the
ABI info is problematic because it requires using the exact same
versions of these tools on build servers as developers' local
build environments to get binary caching to use cached packages
built on a server.
@ras0219-msft
Copy link
Contributor

We've merged #223; we're hesitant to exclude CMake and PowerShell from the abi hashes because they have proven in the past to be significant in non-backwards-compatible ways.

Thanks for the PR! I'm closing since I believe that's the resolution you'd like based on the OP.

@Be-ing
Copy link
Author

Be-ing commented Nov 15, 2021

I'm okay with this being closed but I don't see that #223 was merged?

@Be-ing
Copy link
Author

Be-ing commented Nov 15, 2021

Oh I see that #234 was merged instead. I was not aware of that PR. I would have appreciated a comment on #223 notifying me there was a PR opened for an alternative solution.

@ras0219-msft
Copy link
Contributor

So sorry! I didn't look closely enough at the numbers. Does #234 address your concerns sufficiently?

@Be-ing
Copy link
Author

Be-ing commented Nov 15, 2021

#234 does address my concerns but I still think it is not a great user experience to have to disable two options to get caching working as expected. Refer to my comment on #234 for more; let's continue discussing it there.

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