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

Fix CI crash due to missing package ABIs #331

Closed
wants to merge 1 commit into from

Conversation

ras0219-msft
Copy link
Contributor

@ras0219-msft ras0219-msft commented Jan 24, 2022

PR #298 introduced a regression during ABI calculation for packages
that do not have features. It removed the implicit entry "core"
from InstallPlanAction::feature_list, which meant that packages
without features would end up with an empty ABI tag for their
features list.

We consider an empty ABI tag to indicate that
a value could not be calculated and therefore that the package
cannot be cached (no ABI). This then resulted in a crash during CI
which assumes that all packages have ABIs.

PR microsoft#298 introduced a regression during ABI calculation for packages
that do not have features. It removed the implicit entry "core"
from InstallPlanAction::feature_list, which meant that packages
without features would end up with an empty ABI tag for their
features list.

We consider an empty ABI tag in the features list to indicate that
a value could not be calculated and therefore that the package
cannot be cached (no ABI). This then resulted in a crash during CI
which assumes that all packages have ABIs.

for (size_t i = 0; i < actions.size(); ++i)
{
if (cache_status[i] == nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than here, I think this should be added to https://github.com/microsoft/vcpkg-tool/pull/331/files#diff-045a3245d6bccf63618bd35376d0a973af7805c6e232bbbf222ca326b60b7000L1422

Ever asking the binary cache about a package without an ABI hash is a bug and should blow up.

@BillyONeal
Copy link
Member

I tried adding an assert that core was not turned on here in testing and it fires because all normal ways we got here added core; so I think the right fix was actually #329. See this function which always puts "core" on:

static InternalFeatureSet normalize_feature_list(View<std::string> fs, ImplicitDefault id)

Robert and I discussed this offline and he agreed.

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