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

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

Merged
merged 13 commits into from
Mar 9, 2024

Conversation

BillyONeal
Copy link
Member

No description provided.

@BillyONeal BillyONeal changed the title "<unknown>" is not an origin "<unknown>" and "<command>" are not origins Feb 21, 2024
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
Copy link
Member Author

image

@BillyONeal
Copy link
Member Author

image

ExpectedL<ParsedQualifiedSpecifier> parse_qualified_specifier(StringView input);
Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(ParserBase& parser);
}
ExpectedL<ParsedQualifiedSpecifier> parse_qualified_specifier(StringView input,
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. 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.
  2. There are callers who explicitly want the non-'finalized' version, such as:

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),
};
});

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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?

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

LocalizedString help_topics();

LocalizedString help_topic_valid_triplet(const TripletDatabase& database)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

@BillyONeal BillyONeal merged commit 324c38b into microsoft:main Mar 9, 2024
5 checks passed
@BillyONeal BillyONeal deleted the no-unknown branch March 9, 2024 01:07
{
if (check_origin->empty())
{
Checks::unreachable(VCPKG_LINE_INFO, "origin should not be empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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.

3 participants