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 check to ensure loaded port matches the declared version #347

Merged

Conversation

ras0219-msft
Copy link
Contributor

@ras0219-msft ras0219-msft commented Feb 9, 2022

Fixes microsoft/vcpkg#20503.

The fundamental issue in the thread was that the user's versions database did not match the actual retrieved files. This PR addresses the problem by adding an integrity check to ensure that files retrieved from a registry match the version declared in that registry's database files.

Before:

❯ vcpkg install --triplet x64-windows
Error: Cycle detected during arrow:x64-windows:
bloom-filter:x64-windows

After:

❯ vcpkg install --triplet x64-windows
Error: Failed to load port from D:\src\cycle_detected\./vcpkg_registry\ports/arrow: version specs did not match: 'arrow@6.0.0.20210925#4' != 'arrow@6.0.0.20210925'
Error: Failed to load port from D:\src\cycle_detected\./vcpkg_registry\ports/arrow: version specs did not match: 'arrow@6.0.0.20210925#4' != 'arrow@6.0.0.20210925'

The error is emitted twice because it is checked every time the algorithm attempts to load the file. The errors are accumulated and presented at once, which currently results in duplication.

Potential improvements to this PR:

  • Add an e2e test
  • Don't print duplicate errors
  • Localize

@BillyONeal
Copy link
Member

I think investigating what happened was high priority but this has been in the product for more than a year, so I don't think avoiding adding the e2e test to get it a day or 2 earlier is reasonable.

As for not printing duplicates, isn't that a call to Util::sort_unique_erase away?

@BillyONeal
Copy link
Member

As for not printing duplicates, isn't that a call to Util::sort_unique_erase away?

It was, and I pushed that.

@BillyONeal
Copy link
Member

Localizing the error message changed the content which made the e2e test fail, I pushed a fix.

@ras0219-msft ras0219-msft merged commit dac008b into microsoft:main Feb 11, 2022
@ras0219-msft ras0219-msft deleted the dev/roschuma/registry-integrity branch February 11, 2022 04:29
strega-nil-ms pushed a commit to strega-nil/vcpkg-tool that referenced this pull request Feb 12, 2022
…ft#347)

* Add check to ensure loaded port matches the declared version

* Dedupe errors.

* Add end to end regression test.

* Localize error

* Remove e2e test dependency on Microsoft/vcpkg

* Fixup localized error check.

* Fix format-cxxcode on VS2022 machines, add message map.

Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
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.

cycle detected when using custom vcpkg registration with shared dependencies
3 participants