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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
95 changes: 95 additions & 0 deletions azure-pipelines/end-to-end-tests-dir/cli.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,98 @@ if (-Not ($out.StartsWith('error: unexpected switch: --not-a-switch')))
{
throw 'Bad install --not-a-switch output'
}

$out = Run-VcpkgAndCaptureOutput -TestArgs @('install', 'this-is-super-not-a-#port')
Throw-IfNotFailed

[string]$expected = @"
error: expected the end of input parsing a package spec; this usually means the indicated character is not allowed to be in a package spec. Port, triplet, and feature names are all lowercase alphanumeric+hypens.
on expression: this-is-super-not-a-#port
^

"@

if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw 'Bad malformed port name output; it was: ' + $out
}

$out = Run-VcpkgAndCaptureOutput -TestArgs @('install', 'zlib', '--binarysource=clear;not-a-backend')
Throw-IfNotFailed

$expected = @"
error: unknown binary provider type: valid providers are 'clear', 'default', 'nuget', 'nugetconfig', 'nugettimeout', 'interactive', 'x-azblob', 'x-gcs', 'x-aws', 'x-aws-config', 'http', and 'files'
on expression: clear;not-a-backend
^

"@

if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw 'Bad malformed --binarysource output; it was: ' + $out
}

$out = Run-VcpkgAndCaptureStdErr -TestArgs @('x-package-info', 'zlib#notaport', '--x-json', '--x-installed')
Throw-IfNotFailed
$expected = @"
error: expected an explicit triplet
on expression: zlib#notaport
^

"@
if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw ('Bad error output: ' + $out)
}

$out = Run-VcpkgAndCaptureStdErr -TestArgs @('x-package-info', 'zlib', '--x-json', '--x-installed')
Throw-IfNotFailed
$expected = @"
error: expected an explicit triplet
on expression: zlib
^

"@
if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw ('Bad error output: ' + $out)
}

$out = Run-VcpkgAndCaptureStdErr -TestArgs @('x-package-info', 'zlib:x64-windows[core]', '--x-json', '--x-installed')
Throw-IfNotFailed
$expected = @"
error: expected the end of input parsing a package spec; did you mean zlib[core]:x64-windows instead?
on expression: zlib:x64-windows[core]
^

"@
if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw ('Bad error output: ' + $out)
}

$out = Run-VcpkgAndCaptureStdErr -TestArgs @('x-package-info', 'zlib[core]:x64-windows', '--x-json', '--x-installed')
Throw-IfNotFailed
$expected = @"
error: List of features is not allowed in this context
on expression: zlib[core]:x64-windows
^

"@
if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw ('Bad error output: ' + $out)
}

$out = Run-VcpkgAndCaptureStdErr -TestArgs @('x-package-info', 'zlib:x64-windows(windows)', '--x-json', '--x-installed')
Throw-IfNotFailed
$expected = @"
error: Platform qualifier is not allowed in this context
on expression: zlib:x64-windows(windows)
^

"@
if (-Not ($out.Replace("`r`n", "`n").EndsWith($expected)))
{
throw ('Bad error output: ' + $out)
}
1 change: 1 addition & 0 deletions include/vcpkg/base/jsonreader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.


private:
template<class Type>
Expand Down
29 changes: 20 additions & 9 deletions include/vcpkg/base/message-data.inc.h
Original file line number Diff line number Diff line change
Expand Up @@ -1159,14 +1159,12 @@ DECLARE_MESSAGE(ExpectedDigitsAfterDecimal, (), "", "Expected digits after the d
DECLARE_MESSAGE(ExpectedFailOrSkip, (), "", "expected 'fail', 'skip', or 'pass' here")
DECLARE_MESSAGE(ExpectedFeatureListTerminal, (), "", "expected ',' or ']' in feature list")
DECLARE_MESSAGE(ExpectedFeatureName, (), "", "expected feature name (must be lowercase, digits, '-')")
DECLARE_MESSAGE(ExpectedEof, (), "", "expected eof")
DECLARE_MESSAGE(ExpectedExplicitTriplet, (), "", "expected an explicit triplet")
DECLARE_MESSAGE(ExpectedOneSetOfTags,
(msg::count, msg::old_value, msg::new_value, msg::value),
"{old_value} is a left tag and {new_value} is the right tag. {value} is the input.",
"Found {count} sets of {old_value}.*{new_value} but expected exactly 1, in block:\n{value}")
DECLARE_MESSAGE(ExpectedOneVersioningField, (), "", "expected only one versioning field")
DECLARE_MESSAGE(ExpectedPackageSpecifier, (), "", "expected a package specifier")
DECLARE_MESSAGE(ExpectedPathToExist, (msg::path), "", "Expected {path} to exist after fetching")
DECLARE_MESSAGE(ExpectedPortName, (), "", "expected a port name here (must be lowercase, digits, '-')")
DECLARE_MESSAGE(ExpectedReadWriteReadWrite, (), "", "unexpected argument: expected 'read', readwrite', or 'write'")
Expand Down Expand Up @@ -1684,6 +1682,7 @@ DECLARE_MESSAGE(
"value {path}. To suppress this message, unset the environment variable or use the --vcpkg-root command line "
"switch.")
DECLARE_MESSAGE(IllegalFeatures, (), "", "List of features is not allowed in this context")
DECLARE_MESSAGE(IllegalTriplet, (), "", "Triplet is not allowed in this context")
DECLARE_MESSAGE(IllegalPlatformSpec, (), "", "Platform qualifier is not allowed in this context")
DECLARE_MESSAGE(ImproperShaLength, (msg::value), "{value} is a sha.", "SHA512's must be 128 hex characters: {value}")
DECLARE_MESSAGE(IncorrectArchiveFileSignature, (), "", "Incorrect archive file signature")
Expand Down Expand Up @@ -1858,10 +1857,7 @@ DECLARE_MESSAGE(InvalidCharacterInFeatureName,
(),
"",
"invalid character in feature name (must be lowercase, digits, '-')")
DECLARE_MESSAGE(InvalidCharacterInPackageName,
(),
"",
"invalid character in package name (must be lowercase, digits, '-')")
DECLARE_MESSAGE(InvalidCharacterInPortName, (), "", "invalid character in port name (must be lowercase, digits, '-')")
DECLARE_MESSAGE(InvalidCodePoint, (), "", "Invalid code point passed to utf8_encoded_code_point_count")
DECLARE_MESSAGE(InvalidCodeUnit, (), "", "invalid code unit")
DECLARE_MESSAGE(InvalidCommandArgSort,
Expand Down Expand Up @@ -2220,6 +2216,13 @@ DECLARE_MESSAGE(ParseIdentifierError,
"{value} is a lowercase identifier like 'boost'",
"\"{value}\" is not a valid identifier. "
"Identifiers must be lowercase alphanumeric+hypens and not reserved (see {url} for more information).")
DECLARE_MESSAGE(
ParsePackageNameNotEof,
(msg::url),
"",
"expected the end of input parsing a package name; this usually means the indicated character is not allowed to be "
"in a port name. Port names are all lowercase alphanumeric+hypens and not reserved (see {url} for more "
"information).")
DECLARE_MESSAGE(
ParsePackageNameError,
(msg::package_name, msg::url),
Expand All @@ -2232,6 +2235,16 @@ DECLARE_MESSAGE(ParsePackagePatternError,
"\"{package_name}\" is not a valid package pattern. "
"Package patterns must use only one wildcard character (*) and it must be the last character in "
"the pattern (see {url} for more information).")
DECLARE_MESSAGE(
ParseQualifiedSpecifierNotEof,
(),
"",
"expected the end of input parsing a package spec; this usually means the indicated character is not allowed to be "
"in a package spec. Port, triplet, and feature names are all lowercase alphanumeric+hypens.")
DECLARE_MESSAGE(ParseQualifiedSpecifierNotEofSquareBracket,
(msg::version_spec),
"",
"expected the end of input parsing a package spec; did you mean {version_spec} instead?")
DECLARE_MESSAGE(PathMustBeAbsolute,
(msg::path),
"",
Expand Down Expand Up @@ -2655,7 +2668,6 @@ DECLARE_MESSAGE(UnexpectedEOFMidUnicodeEscape, (), "", "Unexpected end of file i
DECLARE_MESSAGE(UnexpectedErrorDuringBulkDownload, (), "", "an unexpected error occurred during bulk download.")
DECLARE_MESSAGE(UnexpectedEscapeSequence, (), "", "Unexpected escape sequence continuation")
DECLARE_MESSAGE(UnexpectedExtension, (msg::extension), "", "Unexpected archive extension: '{extension}'.")
DECLARE_MESSAGE(UnexpectedFeatureList, (), "", "unexpected list of features")
DECLARE_MESSAGE(UnexpectedField, (msg::json_field), "", "unexpected field '{json_field}'")
DECLARE_MESSAGE(UnexpectedFieldSuggest,
(msg::json_field, msg::value),
Expand All @@ -2669,7 +2681,6 @@ DECLARE_MESSAGE(UnexpectedOption,
(msg::option),
"Option is a command line option like --option=value",
"unexpected option: {option}")
DECLARE_MESSAGE(UnexpectedPlatformExpression, (), "", "unexpected platform expression")
DECLARE_MESSAGE(UnexpectedPortName,
(msg::expected, msg::actual, msg::path),
"{expected} is the expected port and {actual} is the port declared by the user.",
Expand Down Expand Up @@ -2698,7 +2709,7 @@ DECLARE_MESSAGE(UnknownBinaryProviderType,
(),
"",
"unknown binary provider type: valid providers are 'clear', 'default', 'nuget', "
"'nugetconfig','nugettimeout', 'interactive', 'x-azblob', 'x-gcs', 'x-aws', "
"'nugetconfig', 'nugettimeout', 'interactive', 'x-azblob', 'x-gcs', 'x-aws', "
"'x-aws-config', 'http', and 'files'")
DECLARE_MESSAGE(UnknownBooleanSetting,
(msg::option, msg::value),
Expand Down
4 changes: 2 additions & 2 deletions include/vcpkg/base/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ namespace vcpkg

struct ParserBase
{
ParserBase(StringView text, StringView origin, TextRowCol init_rowcol = {});
ParserBase(StringView text, Optional<StringView> origin, TextRowCol init_rowcol = {});

static constexpr bool is_whitespace(char32_t ch) { return ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n'; }
static constexpr bool is_lower_alpha(char32_t ch) { return ch >= 'a' && ch <= 'z'; }
Expand Down Expand Up @@ -119,7 +119,7 @@ namespace vcpkg
int m_column;

StringView m_text;
StringView m_origin;
Optional<StringView> m_origin;

ParseMessages m_messages;
};
Expand Down
2 changes: 1 addition & 1 deletion include/vcpkg/commands.help.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ namespace vcpkg

void command_help_and_exit(const VcpkgCmdArguments& args, const VcpkgPaths& paths);

void help_topic_valid_triplet(const TripletDatabase& database);
void append_help_topic_valid_triplet(LocalizedString& result, const TripletDatabase& database);
}
1 change: 1 addition & 0 deletions include/vcpkg/documentation.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace vcpkg
{
inline constexpr StringLiteral registries_url = "https://learn.microsoft.com/vcpkg/users/registries";
inline constexpr StringLiteral manifests_url = "https://learn.microsoft.com/vcpkg/users/manifests";
inline constexpr StringLiteral package_name_url = "https://learn.microsoft.com/vcpkg/reference/vcpkg-json#name";
inline constexpr StringLiteral assetcaching_url = "https://learn.microsoft.com/vcpkg/users/assetcaching";
inline constexpr StringLiteral binarycaching_url = "https://learn.microsoft.com/vcpkg/users/binarycaching";
inline constexpr StringLiteral versioning_url = "https://learn.microsoft.com/vcpkg/users/versioning";
Expand Down
23 changes: 21 additions & 2 deletions include/vcpkg/fwd/packagespec.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,27 @@ namespace vcpkg
{
enum class ImplicitDefault : bool
{
NO,
YES,
No,
Yes,
};

enum class AllowFeatures : bool
{
No,
Yes,
};

enum class ParseExplicitTriplet
{
Forbid,
Allow,
Require,
};

enum class AllowPlatformSpec : bool
{
No,
Yes,
};

struct PackageSpec;
Expand Down
22 changes: 10 additions & 12 deletions include/vcpkg/input.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include <vcpkg/fwd/triplet.h>
#include <vcpkg/fwd/vcpkgpaths.h>

#include <vcpkg/base/expected.h>

namespace vcpkg
{
// Parse a package spec without features; typically used by commands which
Expand All @@ -15,24 +17,20 @@ namespace vcpkg
// Does not assert that the package spec has a valid triplet. This allows
// such commands to refer to entities that were installed with an overlay
// triplet or similar which is no longer active.
PackageSpec parse_package_spec(StringView spec_string,
Triplet default_triplet,
const LocalizedString& example_text);
[[nodiscard]] ExpectedL<PackageSpec> parse_package_spec(StringView spec_string, Triplet default_triplet);

// Same as the above but checks the validity of the triplet.
PackageSpec check_and_get_package_spec(StringView spec_string,
Triplet default_triplet,
const LocalizedString& example_text,
const TripletDatabase& database);
[[nodiscard]] ExpectedL<PackageSpec> check_and_get_package_spec(StringView spec_string,
Triplet default_triplet,
const TripletDatabase& database);

// Parse a package spec with features, typically used by commands which will
// install or modify a port.
//
// Asserts that the package spec has a valid triplet.
FullPackageSpec check_and_get_full_package_spec(StringView spec_string,
Triplet default_triplet,
const LocalizedString& example_text,
const TripletDatabase& database);
[[nodiscard]] ExpectedL<FullPackageSpec> check_and_get_full_package_spec(StringView spec_string,
Triplet default_triplet,
const TripletDatabase& database);

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.

[[nodiscard]] ExpectedL<Unit> check_triplet(StringView name, const TripletDatabase& database);
}
19 changes: 13 additions & 6 deletions include/vcpkg/packagespec.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,17 +142,24 @@ namespace vcpkg
Optional<PlatformExpression::Expr> platform;

/// @param id add "default" if "core" is not present
/// @return nullopt on success. On failure, caller should supplement returned string with more context.
ExpectedL<FullPackageSpec> to_full_spec(Triplet default_triplet, ImplicitDefault id) const;
// Assumes AllowPlatformSpec::No
FullPackageSpec to_full_spec(Triplet default_triplet, ImplicitDefault id) const;

ExpectedL<PackageSpec> to_package_spec(Triplet default_triplet) const;
// Assumes AllowFeatures::No, AllowPlatformSpec::No
PackageSpec to_package_spec(Triplet default_triplet) const;
};

Optional<std::string> parse_feature_name(ParserBase& parser);
Optional<std::string> parse_package_name(ParserBase& parser);
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),
};
});

AllowFeatures allow_features,
ParseExplicitTriplet parse_explicit_triplet,
AllowPlatformSpec allow_platform_spec);
Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(ParserBase& parser,
AllowFeatures allow_features,
ParseExplicitTriplet parse_explicit_triplet,
AllowPlatformSpec allow_platform_spec);
} // namespace vcpkg

template<>
struct std::hash<vcpkg::PackageSpec>
Expand Down
15 changes: 9 additions & 6 deletions include/vcpkg/paragraphparser.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <vcpkg/base/expected.h>
#include <vcpkg/base/messages.h>
#include <vcpkg/base/optional.h>
#include <vcpkg/base/stringview.h>

#include <vcpkg/packagespec.h>
Expand All @@ -18,6 +19,8 @@ namespace vcpkg
{
using Paragraph = std::map<std::string, std::pair<std::string, TextRowCol>, std::less<>>;

using FieldValue = std::pair<std::string, TextRowCol>;

struct ParagraphParser
{
ParagraphParser(StringView origin, Paragraph&& fields)
Expand All @@ -27,8 +30,8 @@ namespace vcpkg

std::string required_field(StringLiteral fieldname);

std::string optional_field(StringLiteral fieldname);
std::string optional_field(StringLiteral fieldname, TextRowCol& position);
Optional<FieldValue> optional_field(StringLiteral fieldname);
std::string optional_field_or_empty(StringLiteral fieldname);

void add_error(TextRowCol position, msg::MessageT<> error_content);

Expand All @@ -41,9 +44,9 @@ namespace vcpkg
};

ExpectedL<std::vector<std::string>> parse_default_features_list(const std::string& str,
StringView origin = "<unknown>",
TextRowCol textrowcol = {});
Optional<StringView> origin,
TextRowCol textrowcol);
ExpectedL<std::vector<ParsedQualifiedSpecifier>> parse_qualified_specifier_list(const std::string& str,
StringView origin = "<unknown>",
TextRowCol textrowcol = {});
Optional<StringView> origin,
TextRowCol textrowcol);
}