"<unknown>" and "<command>" are not origins #1346

BillyONeal · 2024-02-17T04:35:01Z

No description provided.

azure-pipelines/end-to-end-tests-dir/cli.ps1

The 'vcpkg install' blobs at the end aren't helpful. ```console PS D:\vcpkg-tool\out\build\Win-x64-Debug-WithArtifacts> .\vcpkg.exe install this-is-super-not-a-port#port error: expected eof on expression: this-is-super-not-a-port#port ^ vcpkg install <port name> <port name>... vcpkg install zlib zlib:x64-windows curl boost ```

# Conflicts: # azure-pipelines/end-to-end-tests-dir/cli.ps1 # include/vcpkg/input.h # src/vcpkg/commands.build-external.cpp # src/vcpkg/commands.build.cpp # src/vcpkg/commands.check-support.cpp # src/vcpkg/commands.depend-info.cpp # src/vcpkg/commands.export.cpp # src/vcpkg/commands.install.cpp # src/vcpkg/commands.remove.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/commands.upgrade.cpp # src/vcpkg/input.cpp

BillyONeal · 2024-02-28T04:37:03Z

BillyONeal · 2024-02-28T05:30:10Z

… results.

ras0219-msft · 2024-03-01T19:26:36Z

include/vcpkg/packagespec.h

-    ExpectedL<ParsedQualifiedSpecifier> parse_qualified_specifier(StringView input);
-    Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(ParserBase& parser);
-}
+    ExpectedL<ParsedQualifiedSpecifier> parse_qualified_specifier(StringView input,


Because all of the "finalizer" functions on ParsedQualifiedSpecifier have been changed to strict contracts, I don't think this intermediate is providing any value to consumers.

It seems like it would be better to just remove them and supply type-safe free functions:

ExpectedL<FullPackageSpec> parse_full_spec(StringView input, AllowFeatures allow_features, ParseExplicitTriplet parse_explicit_triplet, ImplicitDefault id, Triplet default_triplet); ExpectedL<PackageSpec> parse_full_spec(StringView input, ParseExplicitTriplet parse_explicit_triplet, Triplet default_triplet);

(This could be done in a follow-up PR)

I agree and tried to do this, but there are 2 problems:

The existing separation is still used to allow the StringView input and ParserBase& input overloads. I tried to change everything to StringView in this work but sadly the ParserBase overload has good reasons to be there.

There are callers who explicitly want the non-'finalized' version, such as:

vcpkg-tool/src/vcpkg/binaryparagraph.cpp

Lines 46 to 54 in 56b4a5b

parse_qualified_specifier_list(parser.optional_field(ParagraphIdDepends)).value_or_exit(VCPKG_LINE_INFO),

[my_triplet](const ParsedQualifiedSpecifier& dep) {

// for compatibility with previous vcpkg versions, we discard all irrelevant information

return PackageSpec{

dep.name,

dep.triplet.map([](auto&& s) { return Triplet::from_canonical_name(std::string(s)); })

.value_or(my_triplet),

};

});

ras0219-msft · 2024-03-01T19:39:54Z

include/vcpkg/base/jsonreader.h

@@ -58,6 +58,7 @@ namespace vcpkg::Json
        const std::vector<LocalizedString>& warnings() const { return m_warnings; }

        std::string path() const noexcept;
+        StringView origin() const noexcept;


Why is origin for ParserBase an Optional<StringView> but for JsonReader a StringView?

ParserBase is reasonably used for things that don't have origins, like content from environment variables or command lines. At least at present, JSON only comes from honest to goodness files which always have origins, so making the parameter nullable is unnecessary.

ras0219-msft · 2024-03-01T19:44:34Z

src/vcpkg/binaryparagraph.cpp

-                                         .value_or_exit(VCPKG_LINE_INFO);
+            TextRowCol defaultFeaturesRowCol;
+            this->default_features =
+                parse_default_features_list(parser.optional_field(Fields::DEFAULT_FEATURES, defaultFeaturesRowCol),


Another case of argument evaluation order UB. I think the .optional_field type signature is prone to this problem; would it make sense to rename the function and return a pair instead?

ras0219-msft · 2024-03-01T19:49:52Z

src/vcpkg/commands.help.cpp

@@ -23,6 +23,13 @@ namespace

    LocalizedString help_topics();

+    LocalizedString help_topic_valid_triplet(const TripletDatabase& database)


This function used to print the help to the screen, but now returns a result. This is very likely to make all consumers silently incorrect -- I see two cases in this file. This should either be changed back to the previous behavior or be attributed with [[nodiscard]] to help catch the cases.

Hmmm I meant to revert interface changes to this function, will do that.

ras0219-msft · 2024-03-01T19:57:12Z

include/vcpkg/input.h


-    void check_triplet(StringView name, const TripletDatabase& database);


Since this function is changing from a side effect to a return value, I think it should at least gain a [[nodiscard]] or (preferably) change its name. All callers must change anyway to use the returned value, so it seems very low cost to also change the identifier.

# Conflicts: # include/vcpkg/documentation.h # src/vcpkg/binaryparagraph.cpp # src/vcpkg/packagespec.cpp # src/vcpkg/paragraphs.cpp

dg0yt · 2024-04-03T05:09:06Z

src/vcpkg/base/parse.cpp

+        {
+            if (check_origin->empty())
+            {
+                Checks::unreachable(VCPKG_LINE_INFO, "origin should not be empty");


Since this change I see this error:

$ /vcpkg format-manifest scripts/test_ports/vcpkg-ci-skia/vcpkg.json /home/<me>/vcpkg-tool/src/vcpkg/base/parse.cpp(106): origin should not be empty Aborted (core dumped)

"<unknown>" is not an origin

58eb07e

BillyONeal mentioned this pull request Feb 17, 2024

Introduce DiagnosticContext, an 'error document' like type. #1323

Open

Merge remote-tracking branch 'origin/main' into no-unknown

bc2ac20

BillyONeal changed the title ~~"<unknown>" is not an origin~~ "<unknown>" and "<command>" are not origins Feb 21, 2024

<command> is not an origin either.

bd20e7a

BillyONeal commented Feb 21, 2024

View reviewed changes

azure-pipelines/end-to-end-tests-dir/cli.ps1 Outdated Show resolved Hide resolved

BillyONeal added 2 commits February 21, 2024 18:27

BillyONeal added 3 commits February 27, 2024 22:17

Improve the error message as requested by @ras0219-msft for malformed…

f3c87e8

… results.

clang-format and warning fix

084839b

More compiler warnings

a5d8d05

ras0219-msft approved these changes Mar 1, 2024

View reviewed changes

BillyONeal added 5 commits March 1, 2024 18:52

Merge remote-tracking branch 'origin/main' into no-unknown

3135acd

# Conflicts: # include/vcpkg/documentation.h # src/vcpkg/binaryparagraph.cpp # src/vcpkg/packagespec.cpp # src/vcpkg/paragraphs.cpp

Merge remote-tracking branch 'origin/main' into no-unknown

1125064

Revert unintentional interface change to help_topic_valid_triplet

097cf87

Shut down the potential optional_field UB.

4ecdba4

Add [[nodiscard]] s

bdeba4d

BillyONeal requested a review from ras0219-msft March 8, 2024 00:05

ras0219-msft approved these changes Mar 9, 2024

View reviewed changes

BillyONeal merged commit 324c38b into microsoft:main Mar 9, 2024
5 checks passed

BillyONeal deleted the no-unknown branch March 9, 2024 01:07

dg0yt reviewed Apr 3, 2024

View reviewed changes

dg0yt mentioned this pull request Apr 8, 2024

<unsaved> is an origin: Fix format-manifest command #1385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"<unknown>" and "<command>" are not origins #1346

"<unknown>" and "<command>" are not origins #1346

BillyONeal commented Feb 17, 2024

BillyONeal commented Feb 28, 2024

BillyONeal commented Feb 28, 2024

ras0219-msft Mar 1, 2024

BillyONeal Mar 8, 2024

ras0219-msft Mar 1, 2024

BillyONeal Mar 7, 2024

ras0219-msft Mar 1, 2024

ras0219-msft Mar 1, 2024

BillyONeal Mar 7, 2024

ras0219-msft Mar 1, 2024

dg0yt Apr 3, 2024

	parse_qualified_specifier_list(parser.optional_field(ParagraphIdDepends)).value_or_exit(VCPKG_LINE_INFO),
	[my_triplet](const ParsedQualifiedSpecifier& dep) {
	// for compatibility with previous vcpkg versions, we discard all irrelevant information
	return PackageSpec{
	dep.name,
	dep.triplet.map([](auto&& s) { return Triplet::from_canonical_name(std::string(s)); })
	.value_or(my_triplet),
	};
	});

		@@ -23,6 +23,13 @@ namespace

		LocalizedString help_topics();

		LocalizedString help_topic_valid_triplet(const TripletDatabase& database)


		void check_triplet(StringView name, const TripletDatabase& database);

"<unknown>" and "<command>" are not origins #1346

"<unknown>" and "<command>" are not origins #1346

Conversation

BillyONeal commented Feb 17, 2024

BillyONeal commented Feb 28, 2024

BillyONeal commented Feb 28, 2024

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