Don't require a name or version in "project" manifests. #605

BillyONeal · 2022-06-25T01:12:31Z

In resolving https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494960 "[vcpkg ce] vcpkg-ce does not find vcpkg.json to find vcpkg-configuration.json" I ran into a problem for existing vcpkg artifacts-only customers, where today they can name artifact dependencies without needing to supply a name or version. I want to be able to provide a reasonable "how to fix it" message for these customers and it seems reasonable to be able to declare dependencies without having a name or version. After some discussion with @ras0219-msft and @vicroms , the requirement for a name and version seems to be a historical artifact of using the same code to parse both port manifests and consumer/project manifests.

include/vcpkg/sourceparagraph.h:
Rename parse_manifest_object to parse_port_manifest_object and add parse_project_manifest_object.

include/vcpkg/versiondeserializers.h:
Declare visit_optional_schemed_deserializer (with static removed in the versiondeserializers.cpp)

include/vcpkg/versions.h:
Add VersionScheme::Missing for the case that it isn't provided.

cmakevars.cpp:
Use "_manifest_" as the "port" name when a name wasn't supplied for purposes of vcpkg_get_dep_info.

add.ps1:
Add a new test of add port that uses an input manifest with no name or version information.

manifests.ps1:
Remove name and version from the prototype manifest. Note that some of the tests still use a manifest with a name; in particular the "self reference" ones so we still have coverage of that.

format-manifest.ps1:
Add project manifest examples in addition to the previous "reserialize all ports" test.

@ras0219-msft

In resolving https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494960 "[vcpkg ce] vcpkg-ce does not find vcpkg.json to find vcpkg-configuration.json" I ran into a problem for existing vcpkg artifacts-only customers, where today they can name artifact dependencies without needing to supply a name or version. I want to be able to provide a reasonable "how to fix it" message for these customers and it seems reasonable to be able to declare dependencies without having a name or version. After some discussion with @ras0219-msft and @vicroms , the requirement for a name and version seems to be a historical artifact of using the same code to parse both port manifests and consumer/project manifests. include/vcpkg/sourceparagraph.h: Rename parse_manifest_object to parse_port_manifest_object and add parse_project_manifest_object. include/vcpkg/versiondeserializers.h: Declare visit_optional_schemed_deserializer (with static removed in the versiondeserializers.cpp) include/vcpkg/versions.h: Add VersionScheme::Missing for the case that it isn't provided. cmakevars.cpp: Use "_manifest_" as the "port" name when a name wasn't supplied for purposes of vcpkg_get_dep_info. add.ps1: Add a new test of add port that uses an input manifest with no name or version information. manifests.ps1: Remove name and version from the prototype manifest. Note that some of the tests still use a manifest with a name; in particular the "self reference" ones so we still have coverage of that. format-manifest.ps1: Add project manifest examples in addition to the previous "reserialize all ports" test.

autoantwort · 2022-06-25T11:59:17Z

Rename parse_manifest_object to parse_port_manifest_object and add parse_project_manifest_object.

Then we can also warn when a user is using overwrites or baseline in a ports manifest.

BillyONeal · 2022-06-27T19:06:56Z

Then we can also warn when a user is using overwrites or baseline in a ports manifest.

We can warn about it but I believe the intent is that those are allowed (and ignored). The idea/feature is that if you're writing a manifest for a port, you can ship the same vcpkg.json with your port and it's OK that we don't obey nontransitive settings there. Our docs could indeed be better about describing what transitive and nontransitive settings are though..

ras0219-msft

LGTM as-is

ras0219-msft · 2022-06-27T18:19:30Z

src/vcpkg-test/manifests.cpp

-        CHECK(pgh.core_paragraph->version_scheme == std::get<1>(v));
-        CHECK(pgh.core_paragraph->raw_version == std::get<2>(v));
-        CHECK(pgh.core_paragraph->port_version == 0);
+        { // project manifest


Suggested change

{ // project manifest

SECTION("project manifest")

{

Hmmm I didn't think SECTION could be inside of a control flow statement like this foreach loop?

I thought it could, but perhaps I'm wrong.

ras0219-msft · 2022-06-27T18:19:49Z

src/vcpkg-test/manifests.cpp

+            CHECK(pgh.core_paragraph->port_version == 0);
+        }
+
+        { // port manifest


Suggested change

{ // port manifest

SECTION("port manifest")

{

ras0219-msft · 2022-06-27T18:28:52Z

src/vcpkg/sourceparagraph.cpp

-            spgh->raw_version = schemed_version.version.text();
-            spgh->version_scheme = schemed_version.scheme;
-            spgh->port_version = schemed_version.version.port_version();
+            parse_name_version(r, obj, spgh);


Instead of using a common entry point that virtually branches into the customization, what about using separate entry points that then call (statically) into the common code?

Have ProjectManifestDeserializer and PortManifestDeserializer define visit_object() and switch this function to something like void visit_common(Json::Reader& r, Json::Object& obj, SourceControlFile& control_file).

This would reduce the amount of indirect branches from 2 to 1 (not for performance, but for clearer flow-of-code understanding).

ras0219-msft · 2022-06-27T18:34:25Z

src/vcpkg-test/manifests.cpp

+            static constexpr StringLiteral zlib_name =
+                R"json(    "name": "zlib",)json" "\n";
+            // clang-format on
+            auto with_name_removed = Strings::replace_all(std::get<0>(v).to_string(), zlib_name, "");


Instead of string replacing here (where it needs to match every test document exactly), I think it would be better to parse to json, then remove the name key-value, then either have the test operate on the json or just re-serialize it.

ras0219-msft · 2022-06-28T14:00:00Z

src/vcpkg/sourceparagraph.cpp

-
-        virtual Optional<std::unique_ptr<SourceControlFile>> visit_object(Json::Reader& r,
-                                                                          const Json::Object& obj) override
+        vcpkg::Optional<std::unique_ptr<vcpkg::SourceControlFile>> visit_object_common(


I'm not thrilled about this returning the Optional<>; I think it's very confusing that the function has a bunch of side effects and then always just moves its last argument into the return.

I think it would be clearer at the call site to have

visit_object_common(obj, spgh, r, *control_file); return std::move(control_file);

versus

return visit_object_common(obj, spgh, r, control_file);

strega-nil · 2022-06-28T19:13:52Z

As the designer of this feature, I don't personally like this. The design of manifests was always such that one would eventually be able to use a simple repository of code as a dependency, and this breaks that feature, imo.

I will point out that both Cargo and npm both require a package name and a version.

However, I'm no longer on the team, so my opinion is obviously less important. Up to y'all.

BillyONeal · 2022-06-28T19:51:23Z

As the designer of this feature, I don't personally like this. The design of manifests was always such that one would eventually be able to use a simple repository of code as a dependency, and this breaks that feature, imo.

Yeah, we absolutely should acknowledge that this weakens the "vcpkg.json is what you publish" story and we need to make sure we are OK with that.

I think for things that are libraries, anything that we would normally package up as a port, we still want documentation etc. to encourage that. If we're integrated into a situation where we know it's a library, e.g. a library vcxproj, it probably makes sense to emit warnings for that, and similar. We also should be making sure vcpkg new is emitting a name and version to encourage supplying them.

However, we also have acknowledged:

"applications" which we explicitly never expect to be turned into ports and consider only as downstream customers, and
monorepo scenarios which need to share a single vcpkg.json to preserve correct link compatibility with features but which would need to be more than one port in practice on publish¹

and for these what name and/or version one is to use is unclear, and forcing someone to provide a nonsense name doesn't seem to further "simple repository of code" for libraries.

I will point out that both Cargo and npm both require a package name and a version.

I can't speak to cargo, but my limited understanding from NPM world is that there all uses are nominally creation of NPM packages: when you distribute your application it's morally the same as the actual NPM package you would make if you were to publish the thing. That means that they assume everything must be packagable and thus everything must obey packaging rules. I don't think we have that assumption, particularly for folks using artifacts only.

However, I'm no longer on the team, so my opinion is obviously less important. Up to y'all.

I certainly value your opinion on this topic; it's why I pinged you out of band for comment. (And am going to do further checking to make sure the team overall have considered this opinion)

¹ Also note that this problem is less bad for ECMAscript since they don't have the link compat problem and different versions/features of ECMAscript packages can exist without stomping on each other as much but https://rushjs.io/pages/advanced/npm_doppelgangers/ are still a problem

ras0219-msft · 2022-06-28T21:33:12Z

It would be great to have a test covering overlay ports / filesystem registries.

We should specifically make sure that failing to specify a name in an overlay is loudly diagnosed.

BillyONeal · 2022-06-29T08:12:28Z

It would be great to have a test covering overlay ports / filesystem registries.

I added one for overlays.

Even looking at docs I don't know how to form a filesystem registry correctly to write a test for that though.

We should specifically make sure that failing to specify a name in an overlay is loudly diagnosed.

(insert rant that I still think appending this message about updating is pointless)

ras0219-msft · 2022-06-29T21:11:52Z

azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1

+
+Run-Vcpkg @commonArgs --overlay-ports="$PSScriptRoot/../e2e_ports/overlays" install broken-no-name
+Throw-IfNotFailed
+Run-Vcpkg @commonArgs --overlay-ports="$PSScriptRoot/../e2e_ports/overlays" install broken-no-version


These should check that the output matches "missing field".

This would diagnose the overlay being ignored as well as failing to output useful error information.

This replaces the TypeScript implementation of new with a C++ one. Other changes: * Now creates both vcpkg.json and vcpkg-configuration.json, which resolves "vcpkg new does not create vcpkg.json" https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494967 * vcpkg-configuration.json can be embedded into vcpkg.json with --single-file. * Now fills in the default registry with the current baseline git SHA. * --name and --version are now mandatory since we are creating vcpkg.json, but they can be skipped with --application as per microsoft#605

* Add Json::stringify overloads without specifying a style. Most callers were just default-initializing JsonStyle which is equivalent to the WithSpaces(2) form. WithSpaces(2) and default-initialized changed to call the new overload(s). Callers who were stringifying 2 Json entities just to compare their strings also dropped the style parameter. Callers who were using anything else were left alone. * Implement new in C++. This replaces the TypeScript implementation of new with a C++ one. Other changes: * Now creates both vcpkg.json and vcpkg-configuration.json, which resolves "vcpkg new does not create vcpkg.json" https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494967 * vcpkg-configuration.json can be embedded into vcpkg.json with --single-file. * Now fills in the default registry with the current baseline git SHA. * --name and --version are now mandatory since we are creating vcpkg.json, but they can be skipped with --application as per #605 * clang-format and messages * Add unit tests. * Add e2e test. * Introduce zero_or_one_set. * This is what tests are for.

DimRochette · 2022-07-27T13:01:08Z

However, we also have acknowledged:
* "applications" which we explicitly never expect to be turned into ports and consider only as downstream customers, and

* monorepo scenarios which need to share a single vcpkg.json to preserve correct link compatibility with features but which would need to be more than one port in practice on publish1
and for these what name and/or version one is to use is unclear, and forcing someone to provide a nonsense name doesn't seem to further "simple repository of code" for libraries.

Our products repo have vcpkg.json with name:"name of repo" version: "0.0.0" because they won't be package with vcpkg ever.
So removing those lines is perfect 👍
I guess most of the userbase of vcpkg will be in that same usecase because creating a package still require lots of investment.

We have private vcpkg registry with library using name and version and that's fair to require it, even if it's painful for us to manage version file of our product=>update version in vcpkg.json of repo to deploy => update version in registry portfile => update version in baseline and try to make them similar to understand version fields in debugging product/CI.

ras0219-msft approved these changes Jun 27, 2022

View reviewed changes

vicroms approved these changes Jun 27, 2022

View reviewed changes

BillyONeal added 4 commits June 27, 2022 16:33

Put the json entity type assert in the right place.

bf71a1d

Use JSON stuff rather than string stuff in the parsing tests.

c6413e7

Invert template method pattern in manifest deserializer.

93a4eac

Formatting

e6308f3

BillyONeal requested a review from ras0219-msft June 28, 2022 07:01

ras0219-msft approved these changes Jun 28, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into manifest-name-version

2236636

Add overlay ports test.

239a98c

ras0219-msft approved these changes Jun 29, 2022

View reviewed changes

Add output asserts.

d07adcf

BillyONeal merged commit b4d664f into microsoft:main Jun 30, 2022

BillyONeal deleted the manifest-name-version branch June 30, 2022 20:10

BillyONeal mentioned this pull request Jul 1, 2022

Implement 'new' in C++. #613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't require a name or version in "project" manifests. #605

Don't require a name or version in "project" manifests. #605

BillyONeal commented Jun 25, 2022 •

edited

Loading

autoantwort commented Jun 25, 2022

BillyONeal commented Jun 27, 2022

ras0219-msft left a comment

ras0219-msft Jun 27, 2022

BillyONeal Jun 27, 2022

ras0219-msft Jun 28, 2022

ras0219-msft Jun 27, 2022

ras0219-msft Jun 27, 2022

BillyONeal Jun 28, 2022

ras0219-msft Jun 27, 2022

BillyONeal Jun 28, 2022

ras0219-msft Jun 28, 2022

strega-nil commented Jun 28, 2022

BillyONeal commented Jun 28, 2022

ras0219-msft commented Jun 28, 2022

BillyONeal commented Jun 29, 2022

ras0219-msft Jun 29, 2022

DimRochette commented Jul 27, 2022

Don't require a name or version in "project" manifests. #605

Don't require a name or version in "project" manifests. #605

Conversation

BillyONeal commented Jun 25, 2022 • edited Loading

autoantwort commented Jun 25, 2022

BillyONeal commented Jun 27, 2022

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil commented Jun 28, 2022

BillyONeal commented Jun 28, 2022

ras0219-msft commented Jun 28, 2022

BillyONeal commented Jun 29, 2022

Choose a reason for hiding this comment

DimRochette commented Jul 27, 2022

BillyONeal commented Jun 25, 2022 •

edited

Loading