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

The great artifacts multi registries overhaul. #690

Merged
merged 27 commits into from
Sep 12, 2022

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Sep 2, 2022

User facing intentional behavior changes:

  • The --registry switch has been removed. There was basically nothing we could do with a registry for which we had no name.
  • Commands which speak registries / IDs now combine the set of names of registries from the global set and the local project set.
  • Console output now always displays registries with the following rules:
  1. If the registry URI is in the project, the name from the project is used.
  2. Otherwise, if the registry URI is is not in the project, and the name of the registry in the global configuration does not conflict with the project, the global configuration's name is used.
  3. Otherwise, the URI enclosed in []s is used. This is a signal to the user that they will need to take some form of manual action.
  • The command x-regenerate now requires a local file path.
  • Several places that used to break when handed relative paths, like x-regenerate, now tolerate this situation correctly.
  • If an artifact declares a dependency with explicit registry, and that registry's name is not declared in that artifact, resolution fails. (We no longer will try to find that name in the project / global configuration)
  • Commands that need activation options like add, and remove, no longer try to activate the project.
  • Artifacts are now installed and otherwise enumerated in topological sorted order. This is the other half of https://devdiv.visualstudio.com/DevDiv/_boards/board/t/Vcpkg%20Package%20Manager/Stories/?workitem=1510278
  • There is no longer a built in registry named 'default'.
  • Except for index.yaml, a few remaining yaml hangers-on have been replaced with JSON. This may invalidate installed artifacts in the wild. This is necessary for the medium-term goal of deleting the YAML implementation from the product entirely.
  • index.yaml no longer begins with 'THIS_IS_NOT_A_MANIFEST_ITS_AN_INDEX_STRING'
  • A bunch of newlines in output are now gone. I can see an argument to add some of these back but we certainly weren't principled about it before and it caused varying numbers of newlines to come out.

Bug fixes:

TODO before un-drafting this review:

  • Fix recording of the 'requested' version in add
  • Audit for FIXMEs
  • Audit for local variable names like 'artifacts' that should now be 'resolutions' or similar.
  • See if the 'initialSelections' machinery in resolveDependencies can be made less painful. I wanted to do this but at this point this work is blocking people so I want to merge what works now. Will probably clean that up a lot in a subsequent review.
  • Separate parts of this into smaller blocks that can be more easily reviewed where that makes sense.
  • Write up better 'line by line' ish commit notes to make it easier to review.
  • Tests

Fun demonstrations

Demonstrating registry naming edge cases:
Registry naming edge cases demonstration

Demonstrating correct registry name and correct dependency star:
Registry name and dependency star demonstration

Demonstrating topological sort. Note that 'headers' is a diamond dependency:

Topological sort demonstration

User facing intentional behavior changes:
* The --registry switch has been removed. There was basically nothing we could do with a registry for which we had no name.
* Commands which speak registries / IDs now combine the set of names of registries from the global set and the local project set.
* Console output now always displays registries with the following rules:
1. If the registry URI is in the project, the name from the project is used.
2. Otherwise, if the registry URI is is not in the project, and the name of the registry in the global configuration does not conflict with the project, the global configuration's name is used.
3. Otherwise, the URI enclosed in []s is used. This is a signal to the user that they will need to take some form of manual action.
* The command x-regenerate now requires a local file path.
* Several places that used to break when handed relative paths, like x-regenerate, now tolerate this situation correctly.
* If an artifact declares a dependency with explicit registry, and that registry's name is not declared in that artifact, resolution fails. (We no longer will try to find that name in the project / global configuration)
* Commands that need activation options like `add`, and `remove`, no longer try to activate the project.
* Artifacts are now installed and otherwise enumerated in topological sorted order. This is the other half of https://devdiv.visualstudio.com/DevDiv/_boards/board/t/Vcpkg%20Package%20Manager/Stories/?workitem=1510278
* There is no longer a built in registry named 'default'.
* Except for index.yaml, a few remaining yaml hangers-on have been replaced with JSON. This may invalidate installed artifacts in the wild. This is necessary for the medium-term goal of deleting the YAML implementation from the product entirely.
* index.yaml no longer begins with 'THIS_IS_NOT_A_MANIFEST_ITS_AN_INDEX_STRING'

Bug fixes:
* The console output no longer gets confused and displays [URL]'d registry names (except as denoted by case (3) above). This was https://devdiv.visualstudio.com/DevDiv/_boards/board/t/Vcpkg%20Package%20Manager/Stories/?workitem=1494994

TODO before un-drafting this review:
* Fix recording of the 'requested' version in `add`
* Audit for FIXMEs
* Audit for local variable names like 'artifacts' that should now be 'resolutions' or similar.
* See if the 'initialSelections' machinery in resolveDependencies can be made less painful.
* Separate parts of this into smaller blocks that can be more easily reviewed where that makes sense.
* Write up better 'line by line' ish commit notes to make it easier to review.
* Tests
@BillyONeal BillyONeal force-pushed the delete-registry-switch branch from b0e3485 to 01d6dcb Compare September 2, 2022 07:49
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Sep 3, 2022
Extracted from microsoft#690

* Deleted a bunch of unreferenced files.
* Fixed files that were CRLF->LF.
* Fixed a few spelling mistakes.
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Sep 3, 2022
BillyONeal added a commit that referenced this pull request Sep 6, 2022
Extracted from #690

* Deleted a bunch of unreferenced files.
* Fixed files that were CRLF->LF.
* Fixed a few spelling mistakes.
BillyONeal added a commit that referenced this pull request Sep 6, 2022
# Conflicts:
#	ce/ce/amf/metadata-file.ts
#	ce/ce/amf/registries.ts
#	ce/ce/cli/styling.ts
#	ce/ce/interfaces/metadata/metadata-format.ts
#	ce/ce/registries/LocalRegistry.ts
#	ce/ce/registries/RemoteRegistry.ts
#	ce/ce/session.ts
#	ce/test/resources/repo/compilers/gnu/gcc-arm-none-eabi-2019.04.0.json
#	ce/test/resources/repo/compilers/gnu/gcc-arm-none-eabi-2019.10.0.json
#	ce/test/resources/repo/compilers/gnu/gcc-arm-none-eabi-2020-10.0.json
#	ce/test/resources/repo/tools/kitware/cmake-3.15.0.json
#	ce/test/resources/repo/tools/kitware/cmake-3.15.1.json
#	ce/test/resources/repo/tools/kitware/cmake-3.17.0.json
#	ce/test/resources/repo/tools/kitware/cmake-3.19.0.json
#	ce/test/resources/repo/tools/kitware/cmake-3.20.0.json
@BillyONeal BillyONeal force-pushed the delete-registry-switch branch from 3bd28e8 to eb07f99 Compare September 6, 2022 20:28
olgaark added a commit to olgaark/vcpkg-ce-catalog that referenced this pull request Sep 7, 2022
The changes in the following files are temporary and need to be reverted after
microsoft/vcpkg-tool#690

crts\microsoft\onecore\x64\x64-14.29.30037.json
crts\microsoft\onecore\x86\x86-14.29.30037.json
crts\microsoft\desktop\x64\x64-14.29.30037.json
crts\microsoft\desktop\x86\x86-14.29.30037.json
@BillyONeal
Copy link
Member Author

Known issue that will not be fixed in this change: version resolution is broken:
image

@BillyONeal BillyONeal marked this pull request as ready for review September 8, 2022 06:05
@BillyONeal BillyONeal merged commit 8569748 into microsoft:main Sep 12, 2022
@BillyONeal BillyONeal deleted the delete-registry-switch branch September 12, 2022 21:07
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