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.

Sign up for GitHub

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

Jump to bottom

[vcpkg registries] Add git registries #15054

Merged
merged 9 commits into from Jan 15, 2021
Merged

[vcpkg registries] Add git registries #15054

merged 9 commits into from Jan 15, 2021

Conversation

strega-nil
Copy link
Contributor

@strega-nil strega-nil commented Dec 11, 2020 •
edited

Add git registries support to the registries module of vcpkg.

Depends on #15583

@PhoebeHui PhoebeHui added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Dec 11, 2020
@strega-nil strega-nil marked this pull request as ready for review December 12, 2020 00:49
ras0219
ras0219 suggested changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

(only reviewed to registries.cpp:193)

This will need a lot of tests added before merging; it should probably pull #15081 to make that easier. I suspect it will be faster to review and merge this PR if it was split into two:

  1. Refactor existing Builtin+Filesystem registry into this new architecture (+tests for those)
  2. Adding Git registries + tests

(Remember that code review costs are super-linear with number of lines changed!)

toolsrc/src/vcpkg/configuration.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/portfileprovider.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/portfileprovider.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Outdated Show resolved Hide resolved
@strega-nil strega-nil mentioned this pull request Dec 14, 2020
Merged
@strega-nil strega-nil marked this pull request as draft December 14, 2020 22:54
@strega-nil strega-nil added the depends:different-pr This PR or Issue depends on a PR which has been filed label Dec 14, 2020
@strega-nil strega-nil changed the title [vcpkg registries] Registries: Take 2 (allow registries + versioning) [vcpkg registries] Add git registries Dec 14, 2020
@vicroms vicroms added the info:internal This PR or Issue was filed by the vcpkg team. label Dec 18, 2020
@strega-nil strega-nil force-pushed the git-registries-impl branch 5 times, most recently from a460826 to 0a9b960 Compare December 21, 2020 23:41
@strega-nil strega-nil removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Dec 21, 2020
@strega-nil strega-nil marked this pull request as ready for review December 21, 2020 23:41
BillyONeal
BillyONeal reviewed Dec 22, 2020
View reviewed changes
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
vicroms
vicroms previously requested changes Dec 23, 2020
View reviewed changes
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
@vicroms vicroms dismissed their stale review January 8, 2021 05:40

Issues solved.

strega-nil
strega-nil commented Jan 8, 2021
View reviewed changes
Copy link
Contributor Author

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I didn't notice anything else in a cursory glance.

scripts/e2e_ports/port_versions/baseline.json Show resolved Hide resolved
ras0219
ras0219 suggested changes Jan 9, 2021
View reviewed changes
Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

This also needs several end-to-end tests. I'd suggest having the end-to-end tests construct a local git repo with an appropriate history and then point to that temporary repo for testing.

toolsrc/src/vcpkg/base/system.process.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/commands.porthistory.cpp Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/system.process.h Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Show resolved Hide resolved
@strega-nil
Copy link
Contributor Author

This also needs several end-to-end tests. I'd suggest having the end-to-end tests construct a local git repo with an appropriate history and then point to that temporary repo for testing.

It does, though? https://github.com/microsoft/vcpkg/pull/15054/files#diff-de6d1117ad4379767b02529167248f18bbad1b56758118cccfaa915d534be7b7R56

@ras0219
Copy link
Contributor

ras0219 commented Jan 11, 2021

Oops, I managed to forget about the e2e tests by the time I got to the end of the diff 👍

@strega-nil strega-nil mentioned this pull request Jan 11, 2021
Merged
@strega-nil strega-nil marked this pull request as draft January 11, 2021 19:04
@strega-nil strega-nil added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jan 11, 2021
@strega-nil strega-nil force-pushed the git-registries-impl branch 2 times, most recently from 248acec to 98f90a7 Compare January 12, 2021 17:44
@strega-nil strega-nil marked this pull request as ready for review January 12, 2021 17:50
BillyONeal
BillyONeal reviewed Jan 13, 2021
View reviewed changes
scripts/azure-pipelines/end-to-end-tests-dir/registries.ps1 Show resolved Hide resolved
scripts/azure-pipelines/end-to-end-tests-dir/registries.ps1 Outdated Show resolved Hide resolved
scripts/azure-pipelines/end-to-end-tests-dir/registries.ps1 Outdated Show resolved Hide resolved
scripts/azure-pipelines/end-to-end-tests-dir/registries.ps1 Show resolved Hide resolved
scripts/azure-pipelines/end-to-end-tests-prelude.ps1 Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/unique_resource.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/unique_resource.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/unique_resource.h Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Outdated Show resolved Hide resolved
@strega-nil strega-nil force-pushed the git-registries-impl branch 2 times, most recently from b92308f to 9e00cc6 Compare January 14, 2021 00:53
@strega-nil
[vcpkg registries] Add git registries support
f8d6c1c 
* Add git registries support to the registries module of vcpkg.
* add e2e tests for git registries
* fix vcpkg.cmake for registries
@strega-nil
fix CRs, remove a thing
8a9951f
@strega-nil
better error messages
7ad514e
@strega-nil
Billy CRs
f73aa64
@strega-nil
fix Robert's CR comment
0052b57
@strega-nil
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil merged commit a597134 into microsoft:master Jan 15, 2021
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Jan 15, 2021
@Jimmy-Hu
Merge pull request #491 from microsoft/master
1de2248 
[vcpkg registries] Add git registries (microsoft#15054)
@strega-nil strega-nil deleted the git-registries-impl branch January 19, 2021 19:51
strega-nil added a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
@strega-nil
[vcpkg registries] Add git registries (microsoft#15054)
8cde8a0 
* [vcpkg registries] Add git registries support

* Add git registries support to the registries module of vcpkg.
* add e2e tests for git registries
* fix vcpkg.cmake for registries

* fix CRs, remove a thing

* better error messages

* Billy CRs

* fix Robert's CR comment

* I learned about `-c` today

* format

* fix baseline.json

* failing to find baseline is technically not a bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@ras0219 ras0219 ras0219 approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

@vicroms vicroms Awaiting requested review from vicroms

Assignees

@PhoebeHui PhoebeHui

Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed depends:different-pr This PR or Issue depends on a PR which has been filed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@strega-nil @ras0219 @BillyONeal @vicroms @PhoebeHui