[vcpkg registries] support versions #15114

strega-nil · 2020-12-14T22:52:09Z

This PR merges the Registries changes and the versioning changes, so that one can use both at the same time.

There is one major difference between this PR and the RFC (#13590), which is that instead of version files looking like:

[
  ...
]

version files look like:

{
  "versions": [
    ...
  ]
}

this is to support interop between this PR and existing demos and the like;
fixing this, along with perhaps renaming port_versions to port-versions should be done after this is merged,
should be a trivial change.

This is the first PR resulting from splitting #15054 into multiple PRs.

The e2e tests for filesystem registries will come in the git registries PR, since there's already test code written for that.

This PR merges the Registries changes and the versioning changes, so that one can use both at the same time. There is one major difference between this PR and the RFC (microsoft#13590), which is that instead of version files looking like: ```json [ ... ] ``` version files look like: ``` { "versions": [ ... ] } ``` this is to support interop between this PR and existing demos and the like; fixing this, along with perhaps renaming `port_versions` to `port-versions` should be done after this is merged, should be a trivial change.

strega-nil

These are the things I noticed and am fixing now, along with some notes.

toolsrc/src/vcpkg/base/files.cpp

toolsrc/src/vcpkg/versiondeserializers.cpp

toolsrc/src/vcpkg/vcpkgpaths.cpp

toolsrc/src/vcpkg/registries.cpp

BillyONeal

I feel bad that I don't have substantial code review comments, because most of this is parsing code etc. and I don't think humans in general are very good at reviewing that without debugging through examples or having complete test coverage.

I do think this needs some tests before it is merged. (I don't if versions had test coverage but had I been around to review that I'd probably have made the same comment :/ )

toolsrc/include/vcpkg/base/files.h

toolsrc/include/vcpkg/base/jsonreader.h

toolsrc/include/vcpkg/configurationdeserializer.h

toolsrc/src/vcpkg/install.cpp

toolsrc/src/vcpkg/portfileprovider.cpp

toolsrc/src/vcpkg/registries.cpp

…ns-merge

found a bug!

ras0219-msft

Review still in progress (portfileprovider.cpp:299)

scripts/e2e_port_versions/baseline.json

toolsrc/include/vcpkg/configuration.h

toolsrc/include/vcpkg/registries.h

toolsrc/include/vcpkg/sourceparagraph.h

toolsrc/include/vcpkg/vcpkgpaths.h

toolsrc/src/vcpkg-test/registries.cpp

toolsrc/src/vcpkg/configuration.cpp

toolsrc/src/vcpkg/portfileprovider.cpp

switch to using the same git archive trick which is used in the git registry pr

BillyONeal

I still would really like to see unit level tests for any parsing code that aren't present here.

toolsrc/src/vcpkg-test/registries.cpp

toolsrc/src/vcpkg/configuration.cpp

toolsrc/src/vcpkg/portfileprovider.cpp

strega-nil · 2020-12-21T22:04:36Z

/azp run

azure-pipelines · 2020-12-21T22:04:47Z

Azure Pipelines successfully started running 1 pipeline(s).

toolsrc/include/vcpkg/vcpkgcmdarguments.h

[vcpkg registries] support versions (microsoft#15114)

ras0219

@strega-nil Please consider opening a new PR that addresses these issues

ras0219 · 2020-12-14T23:24:12Z

toolsrc/src/vcpkg/configuration.cpp


        if (!regs.empty() && !registries_enabled)
        {
            registries_feature_flags_warning = true;
            regs.clear();
        }

+        if (!r.errors().empty())
+        {
+            return nullopt;


nullopt implies to the caller that the current object is completely outside the bounds of the current deserializer to handle -- for example, asking StringDeserializer to operate on null or 10. The caller will then emit a message to the effect of Error: expected $.field to be of type string.

However, in this case, you're (presumably?) trying to test for whether an error was emitted above as part of parsing the optional object fields. If one of those did result in an error, then the user will already see that more specific error and does not need the generic one.

This is fixed by returning a default constructed object of some kind; the code consuming this deserializer should handle this case accordingly (either by testing r for errors if it's outside the deserializer framework or handling the default constructed form if within the deserializer framework).

ras0219 · 2020-12-14T23:25:55Z

toolsrc/src/vcpkg/configuration.cpp


        if (!regs.empty() && !registries_enabled)
        {
            registries_feature_flags_warning = true;
            regs.clear();
        }

+        if (!r.errors().empty())


This is almost certainly not the right condition -- this will test whether any errors have occurred anywhere in the reader to this point, not just in child objects.

If you need to detect errors in child objects and return early, you'll need to check those objects for an appropriate error state (empty collection, etc).

ras0219 · 2020-12-21T19:35:52Z

toolsrc/src/vcpkg/portfileprovider.cpp

-                auto maybe_error =
-                    p->source_control_file->check_against_feature_flags(p->source_location, paths.get_feature_flags());
-                if (maybe_error) return std::move(*maybe_error.get());
+                auto port_path = port->get_path_to_version(paths, port_version).value_or_exit(VCPKG_LINE_INFO);


This should probably propagate the error to the caller instead of crashing (based on the function signature)

ras0219 · 2020-12-21T19:39:06Z

toolsrc/src/vcpkg/portfileprovider.cpp

-            const Optional<std::string> m_baseline;
-            Lazy<Optional<std::map<std::string, VersionT, std::less<>>>> baseline_cache;
-            mutable std::unique_ptr<PathsPortFileProvider> m_portfile_provider;
+            const VcpkgPaths& paths; // TODO: remove this data member


Suggested change

const VcpkgPaths& paths; // TODO: remove this data member

const VcpkgPaths& paths;

Or remove it :)

ras0219 · 2020-12-21T19:40:17Z

toolsrc/src/vcpkg/portfileprovider.cpp

-            Lazy<Optional<std::map<std::string, VersionT, std::less<>>>> baseline_cache;
-            mutable std::unique_ptr<PathsPortFileProvider> m_portfile_provider;
+            const VcpkgPaths& paths; // TODO: remove this data member
+            mutable std::map<std::string, Optional<VersionT>, std::less<>> m_baseline_cache;


This could use vcpkg::Cache<>.

* [vcpkg registries] support versions This PR merges the Registries changes and the versioning changes, so that one can use both at the same time. There is one major difference between this PR and the RFC (microsoft#13590), which is that instead of version files looking like: ```json [ ... ] ``` version files look like: ``` { "versions": [ ... ] } ``` this is to support interop between this PR and existing demos and the like; fixing this, along with perhaps renaming `port_versions` to `port-versions` should be done after this is merged, should be a trivial change.

strega-nil mentioned this pull request Dec 14, 2020

[vcpkg registries] Add git registries #15054

Merged

robert CRs

96b9c30

strega-nil commented Dec 14, 2020

View reviewed changes

strega-nil added 2 commits December 14, 2020 15:12

Fix nicole CRs.

e0b0ddd

fix minor logic bug in overlay ports

b2cdfd1

JonLiu1993 assigned PhoebeHui Dec 15, 2020

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 15, 2020

add a comment explaining GitRegistryEntry

ac58c58

BillyONeal reviewed Dec 15, 2020

View reviewed changes

strega-nil added 2 commits December 16, 2020 11:48

Merge remote-tracking branch 'upstream/master' into registries-versio…

4f36960

…ns-merge

Billy CRs

e1562bf

strega-nil requested a review from BillyONeal December 16, 2020 20:25

strega-nil added 3 commits December 16, 2020 14:42

add unit tests for RegistrySet

7dcc5eb

testing: add e2e testing for registries

fdf651f

Add e2e tests

916f6e7

found a bug!

ras0219-msft reviewed Dec 17, 2020

View reviewed changes

strega-nil added 2 commits December 17, 2020 16:00

Robert CRs, e2e test fixing

9e31ad8

switch to using the same git archive trick which is used in the git registry pr

format

31ea1d5

vicroms added the info:internal This PR or Issue was filed by the vcpkg team. label Dec 18, 2020

add --no-hardlinks

1b8f839

strega-nil force-pushed the registries-versions-merge branch 4 times, most recently from a0bc553 to 4fe7b03 Compare December 18, 2020 22:19

Switch back to old way of doing check out

360217b

strega-nil force-pushed the registries-versions-merge branch from 4fe7b03 to 360217b Compare December 18, 2020 22:29

BillyONeal approved these changes Dec 18, 2020

View reviewed changes

toolsrc/src/vcpkg-test/registries.cpp Show resolved Hide resolved

toolsrc/src/vcpkg/configuration.cpp Outdated Show resolved Hide resolved

toolsrc/src/vcpkg/portfileprovider.cpp Outdated Show resolved Hide resolved

strega-nil added 2 commits December 18, 2020 15:13

billy crs

1ca69d6

format

64556b3

add registry parsing unit tests

260d25e

strega-nil force-pushed the registries-versions-merge branch from 5494373 to 260d25e Compare December 21, 2020 20:07

BillyONeal approved these changes Dec 21, 2020

View reviewed changes

vicroms approved these changes Dec 21, 2020

View reviewed changes

toolsrc/include/vcpkg/vcpkgcmdarguments.h Show resolved Hide resolved

strega-nil merged commit c898283 into microsoft:master Dec 21, 2020

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Dec 22, 2020

Merge pull request #452 from microsoft/master

d72c659

[vcpkg registries] support versions (microsoft#15114)

ras0219 reviewed Dec 22, 2020

View reviewed changes

vicroms mentioned this pull request Jan 5, 2021

[vcpkg] Add commands to maintain and verify versions db integrity #14999

Merged

10 tasks

strega-nil deleted the registries-versions-merge branch January 19, 2021 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg registries] support versions #15114

[vcpkg registries] support versions #15114

strega-nil commented Dec 14, 2020 •

edited

Loading

strega-nil left a comment •

edited

Loading

BillyONeal left a comment

ras0219-msft left a comment

BillyONeal left a comment

strega-nil commented Dec 21, 2020

azure-pipelines bot commented Dec 21, 2020

ras0219 left a comment •

edited

Loading

ras0219 Dec 14, 2020

ras0219 Dec 14, 2020

ras0219 Dec 21, 2020

ras0219 Dec 21, 2020

ras0219 Dec 21, 2020

	const VcpkgPaths& paths; // TODO: remove this data member
	const VcpkgPaths& paths;

[vcpkg registries] support versions #15114

[vcpkg registries] support versions #15114

Conversation

strega-nil commented Dec 14, 2020 • edited Loading

strega-nil left a comment • edited Loading

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

ras0219-msft left a comment

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

strega-nil commented Dec 21, 2020

azure-pipelines bot commented Dec 21, 2020

ras0219 left a comment • edited Loading

Choose a reason for hiding this comment

ras0219 Dec 14, 2020

Choose a reason for hiding this comment

ras0219 Dec 14, 2020

Choose a reason for hiding this comment

ras0219 Dec 21, 2020

Choose a reason for hiding this comment

ras0219 Dec 21, 2020

Choose a reason for hiding this comment

ras0219 Dec 21, 2020

Choose a reason for hiding this comment

strega-nil commented Dec 14, 2020 •

edited

Loading

strega-nil left a comment •

edited

Loading

ras0219 left a comment •

edited

Loading