Skip to content

Always accept = or space as delimiters when parsing common command line parameters. #13857

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.

Sign up for GitHub

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

Merged
BillyONeal merged 9 commits into from
Oct 7, 2020
Merged

Always accept = or space as delimiters when parsing common command line parameters. #13857

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c0787ed
Always accept = or space as delimiters when parsing command line para…
BillyONeal Oct 2, 2020
00a1d8a
Fix vcpkgcmdarguments allowing bogus switches.
BillyONeal Oct 2, 2020
72130ec
Merge remote-tracking branch 'origin/master' into command_line_root
BillyONeal Oct 7, 2020
ae6431f
CR feedback.
BillyONeal Oct 7, 2020
b4a9001
More args=>testArgs fixes.
BillyONeal Oct 7, 2020
3e8975b
Fix inverted if test in e2e tests.
BillyONeal Oct 7, 2020
2a47028
Add return 0 to fix bad Azure Pipelines failure.
BillyONeal Oct 7, 2020
7046e6f
Extract Run-Vcpkg in e2e tests.
BillyONeal Oct 7, 2020
fd809f7
Try to fix $LASTEXITCODE again....
BillyONeal Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 41 additions & 17 deletions scripts/azure-pipelines/end-to-end-tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,13 @@ function Throw-IfFailed {
throw "'$CurrentTest' had a step with a nonzero exit code"
}
}
function Throw-IfNotFailed {
if ($LASTEXITCODE -ne 0) {
throw "'$CurrentTest' had a step with an unexpectedly zero exit code"
}
}

if (-not $IsLinux -and -not $IsMacOS)
{
if (-not $IsLinux -and -not $IsMacOS) {
Refresh-TestRoot
# Test msbuild props and targets
$CurrentTest = "zlib:x86-windows-static msbuild scripts\testing\integrate-install\..."
Expand Down Expand Up @@ -106,26 +110,26 @@ if (-not $IsLinux -and -not $IsMacOS)
Refresh-TestRoot

# Test simple installation
$args = $commonArgs + @("install","rapidjson","--binarycaching","--x-binarysource=clear;files,$ArchiveRoot,write;nuget,$NuGetRoot,readwrite")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("install", "rapidjson", "--binarycaching", "--x-binarysource=clear;files,$ArchiveRoot,write;nuget,$NuGetRoot,readwrite")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfFailed

Require-FileExists "$installRoot/$Triplet/include/rapidjson/rapidjson.h"

# Test simple removal
$args = $commonArgs + @("remove", "rapidjson")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("remove", "rapidjson")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfFailed

Require-FileNotExists "$installRoot/$Triplet/include/rapidjson/rapidjson.h"

# Test restoring from files archive
$args = $commonArgs + @("install","rapidjson","--binarycaching","--x-binarysource=clear;files,$ArchiveRoot,read")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("install", "rapidjson", "--binarycaching", "--x-binarysource=clear;files,$ArchiveRoot,read")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Remove-Item -Recurse -Force $installRoot
Remove-Item -Recurse -Force $buildtreesRoot
Write-Host $CurrentTest
Expand All @@ -136,8 +140,8 @@ Require-FileExists "$installRoot/$Triplet/include/rapidjson/rapidjson.h"
Require-FileNotExists "$buildtreesRoot/rapidjson/src"

# Test restoring from nuget
$args = $commonArgs + @("install","rapidjson","--binarycaching","--x-binarysource=clear;nuget,$NuGetRoot")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("install", "rapidjson", "--binarycaching", "--x-binarysource=clear;nuget,$NuGetRoot")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Remove-Item -Recurse -Force $installRoot
Remove-Item -Recurse -Force $buildtreesRoot
Write-Host $CurrentTest
Expand All @@ -148,8 +152,8 @@ Require-FileExists "$installRoot/$Triplet/include/rapidjson/rapidjson.h"
Require-FileNotExists "$buildtreesRoot/rapidjson/src"

# Test four-phase flow
$args = $commonArgs + @("install","rapidjson","--dry-run","--x-write-nuget-packages-config=$TestingRoot/packages.config")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("install", "rapidjson", "--dry-run", "--x-write-nuget-packages-config=$TestingRoot/packages.config")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Remove-Item -Recurse -Force $installRoot -ErrorAction SilentlyContinue
Write-Host $CurrentTest
./vcpkg @args
Expand All @@ -161,16 +165,17 @@ Require-FileExists "$TestingRoot/packages.config"
if ($IsLinux -or $IsMacOS) {
mono $(./vcpkg fetch nuget) restore $TestingRoot/packages.config -OutputDirectory "$NuGetRoot2" -Source "$NuGetRoot"
Throw-IfFailed
} else {
}
else {
& $(./vcpkg fetch nuget) restore $TestingRoot/packages.config -OutputDirectory "$NuGetRoot2" -Source "$NuGetRoot"
Throw-IfFailed
}

Remove-Item -Recurse -Force $NuGetRoot -ErrorAction SilentlyContinue
mkdir $NuGetRoot

$args = $commonArgs + @("install","rapidjson","tinyxml","--binarycaching","--x-binarysource=clear;nuget,$NuGetRoot2;nuget,$NuGetRoot,write")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("install", "rapidjson", "tinyxml", "--binarycaching", "--x-binarysource=clear;nuget,$NuGetRoot2;nuget,$NuGetRoot,write")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfFailed
Expand All @@ -184,8 +189,8 @@ if ((Get-ChildItem $NuGetRoot -Filter '*.nupkg' | Measure-Object).Count -ne 1) {
}

# Test export
$args = $commonArgs + @("export","rapidjson","tinyxml","--nuget","--nuget-id=vcpkg-export","--nuget-version=1.0.0","--output=vcpkg-export-output","--raw","--zip","--output-dir=$TestingRoot")
$CurrentTest = "./vcpkg $($args -join ' ')"
$testArgs = $commonArgs + @("export", "rapidjson", "tinyxml", "--nuget", "--nuget-id=vcpkg-export", "--nuget-version=1.0.0", "--output=vcpkg-export-output", "--raw", "--zip", "--output-dir=$TestingRoot")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
Require-FileNotExists "$TestingRoot/vcpkg-export-output"
Require-FileNotExists "$TestingRoot/vcpkg-export.1.0.0.nupkg"
Expand All @@ -194,3 +199,22 @@ Require-FileNotExists "$TestingRoot/vcpkg-export-output.zip"
Require-FileExists "$TestingRoot/vcpkg-export-output"
Require-FileExists "$TestingRoot/vcpkg-export.1.0.0.nupkg"
Require-FileExists "$TestingRoot/vcpkg-export-output.zip"

# Test bad command lines
$testArgs = $commonArgs + @("install", "zlib", "--vcpkg-rootttttt", "C:\")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfNotFailed

$testArgs = $commonArgs + @("install", "zlib", "--vcpkg-rootttttt=C:\")
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfNotFailed

$testArgs = $commonArgs + @("install", "zlib", "--fast") # NB: --fast is not a switch
$CurrentTest = "./vcpkg $($testArgs -join ' ')"
Write-Host $CurrentTest
./vcpkg @args
Throw-IfNotFailed
18 changes: 18 additions & 0 deletions toolsrc/src/vcpkg-test/arguments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,21 @@ TEST_CASE ("VcpkgCmdArguments from argument sequence with valued options", "[arg
REQUIRE(v.command_arguments.size() == 0);
}
}

TEST_CASE ("vcpkg_root parse with arg separator", "[arguments]")
{
std::vector<std::string> t = {"--vcpkg-root", "C:\\vcpkg"};
auto v = VcpkgCmdArguments::create_from_arg_sequence(t.data(), t.data() + t.size());
auto& vcpkg_root_dir = v.vcpkg_root_dir;
REQUIRE(vcpkg_root_dir);
REQUIRE(*vcpkg_root_dir == "C:\\vcpkg");
}

TEST_CASE ("vcpkg_root parse with equal separator", "[arguments]")
{
std::vector<std::string> t = {"--vcpkg-root=C:\\vcpkg"};
auto v = VcpkgCmdArguments::create_from_arg_sequence(t.data(), t.data() + t.size());
auto& vcpkg_root_dir = v.vcpkg_root_dir;
REQUIRE(vcpkg_root_dir);
REQUIRE(*vcpkg_root_dir == "C:\\vcpkg");
}
128 changes: 63 additions & 65 deletions toolsrc/src/vcpkg/vcpkgcmdarguments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,6 @@ namespace vcpkg
}
}

static void parse_value(const std::string* arg_begin,
const std::string* arg_end,
StringView option_name,
std::unique_ptr<std::string>& option_field)
{
if (arg_begin == arg_end)
{
System::print2(System::Color::error, "Error: expected value after --", option_name, '\n');
Metrics::g_metrics.lock()->track_property("error", "error option name");
print_usage();
Checks::exit_fail(VCPKG_LINE_INFO);
}

if (option_field != nullptr)
{
System::print2(System::Color::error, "Error: --", option_name, " specified multiple times\n");
Metrics::g_metrics.lock()->track_property("error", "error option specified multiple times");
print_usage();
Checks::exit_fail(VCPKG_LINE_INFO);
}

option_field = std::make_unique<std::string>(*arg_begin);
}

static void parse_cojoined_value(StringView new_value,
StringView option_name,
std::unique_ptr<std::string>& option_field)
Expand Down Expand Up @@ -176,27 +152,43 @@ namespace vcpkg
return VcpkgCmdArguments::create_from_arg_sequence(v.data(), v.data() + v.size());
}

// returns true if this does parse this argument as this option
// returns the number of consumed arguments:
// 0: parse unsuccessful
// 1: parse successful and option was consumed
// 2: parse successful and both option and lookahead were consumed
template<class T, class F>
static bool try_parse_argument_as_option(StringView option, StringView arg, T& place, F parser)
static size_t try_parse_argument_as_option(
Comment thread
strega-nil marked this conversation as resolved.
Outdated
StringView arg, Optional<StringView> lookahead, StringView option, T& place, F parser)
{
if (arg.size() <= option.size() + 1)
{
// it is impossible for this argument to be this option
return false;
}

if (Strings::starts_with(arg, "x-") && !Strings::starts_with(option, "x-"))
{
arg = arg.substr(2);
}
if (Strings::starts_with(arg, option) && arg.byte_at_index(option.size()) == '=')

if (Strings::starts_with(arg, option))
{
parser(arg.substr(option.size() + 1), option, place);
return true;
if (arg.size() == option.size())
{
if (auto next = lookahead.get())
{
parser(*next, option, place);
return 2;
}

System::print2(System::Color::error, "Error: expected value after ", option, '\n');
Metrics::g_metrics.lock()->track_property("error", "error option name");
print_usage();
Checks::exit_fail(VCPKG_LINE_INFO);
}

if (arg.byte_at_index(option.size()) == '=')
{
parser(arg.substr(option.size() + 1), option, place);
return 1;
}
}

return false;
return 0;
}

static bool equals_modulo_experimental(StringView arg, StringView option)
Expand Down Expand Up @@ -230,13 +222,13 @@ namespace vcpkg
return false;
}

VcpkgCmdArguments VcpkgCmdArguments::create_from_arg_sequence(const std::string* arg_begin,
const std::string* arg_end)
VcpkgCmdArguments VcpkgCmdArguments::create_from_arg_sequence(const std::string* arg_first,
const std::string* arg_last)
{
VcpkgCmdArguments args;
std::vector<std::string> feature_flags;

for (auto it = arg_begin; it != arg_end; ++it)
for (auto it = arg_first; it != arg_last; ++it)
{
std::string basic_arg = *it;

Expand Down Expand Up @@ -269,23 +261,10 @@ namespace vcpkg
Strings::ascii_to_lowercase(std::begin(basic_arg), first_eq);
// basic_arg[0] == '-' && basic_arg[1] == '-'
StringView arg = StringView(basic_arg).substr(2);

// command switch
if (arg == VCPKG_ROOT_DIR_ARG)
{
++it;
parse_value(it, arg_end, VCPKG_ROOT_DIR_ARG, args.vcpkg_root_dir);
continue;
}
if (arg == TRIPLET_ARG)
{
++it;
parse_value(it, arg_end, TRIPLET_ARG, args.triplet);
continue;
}

constexpr static std::pair<StringView, std::unique_ptr<std::string> VcpkgCmdArguments::*>
cojoined_values[] = {
{VCPKG_ROOT_DIR_ARG, &VcpkgCmdArguments::vcpkg_root_dir},
{TRIPLET_ARG, &VcpkgCmdArguments::triplet},
{MANIFEST_ROOT_DIR_ARG, &VcpkgCmdArguments::manifest_root_dir},
{BUILDTREES_ROOT_DIR_ARG, &VcpkgCmdArguments::buildtrees_root_dir},
{DOWNLOADS_ROOT_DIR_ARG, &VcpkgCmdArguments::downloads_root_dir},
Expand All @@ -312,30 +291,49 @@ namespace vcpkg
{JSON_SWITCH, &VcpkgCmdArguments::json},
};

Optional<StringView> lookahead;
{
auto next = it;
++next;
if (next != arg_last)
{
lookahead = *next;
}
}
Comment thread
strega-nil marked this conversation as resolved.

bool found = false;
for (const auto& pr : cojoined_values)
{
if (try_parse_argument_as_option(pr.first, arg, args.*pr.second, parse_cojoined_value))
switch (try_parse_argument_as_option(arg, lookahead, pr.first, args.*pr.second, parse_cojoined_value))
{
found = true;
break;
case 2: ++it; // fallthrough
Comment thread
strega-nil marked this conversation as resolved.
Outdated
case 1: found = true; break;
case 0: break;
default: Checks::unreachable(VCPKG_LINE_INFO);
}
}
if (found) continue;

for (const auto& pr : cojoined_multivalues)
{
if (try_parse_argument_as_option(pr.first, arg, args.*pr.second, parse_cojoined_multivalue))
switch (
try_parse_argument_as_option(arg, lookahead, pr.first, args.*pr.second, parse_cojoined_multivalue))
{
found = true;
break;
case 2: ++it; // fallthrough
case 1: found = true; break;
case 0: break;
default: Checks::unreachable(VCPKG_LINE_INFO);
}
}
if (found) continue;

if (try_parse_argument_as_option(FEATURE_FLAGS_ARG, arg, feature_flags, parse_cojoined_list_multivalue))
switch (try_parse_argument_as_option(
arg, lookahead, FEATURE_FLAGS_ARG, feature_flags, parse_cojoined_list_multivalue))
{
continue;
case 2: ++it; // fallthrough
case 1: continue;
case 0: break;
default: Checks::unreachable(VCPKG_LINE_INFO);
}

for (const auto& pr : switches)
Expand Down Expand Up @@ -515,7 +513,7 @@ namespace vcpkg
}
}

if (!switches_copy.empty())
if (!switches_copy.empty() || !options_copy.empty())
{
System::printf(System::Color::error, "Unknown option(s) for command '%s':\n", this->command);
for (auto&& switch_ : switches_copy)
Expand Down Expand Up @@ -609,7 +607,7 @@ namespace vcpkg
return Strings::concat("--", arg, joiner, value);
};

table.format(opt(TRIPLET_ARG, " ", "<t>"), "Specify the target architecture triplet. See 'vcpkg help triplet'");
table.format(opt(TRIPLET_ARG, "=", "<t>"), "Specify the target architecture triplet. See 'vcpkg help triplet'");
table.format("", "(default: " + format_environment_variable("VCPKG_DEFAULT_TRIPLET") + ')');
table.format(opt(OVERLAY_PORTS_ARG, "=", "<path>"), "Specify directories to be used when searching for ports");
table.format("", "(also: " + format_environment_variable("VCPKG_OVERLAY_PORTS") + ')');
Expand All @@ -619,7 +617,7 @@ namespace vcpkg
"Add sources for binary caching. See 'vcpkg help binarycaching'");
table.format(opt(DOWNLOADS_ROOT_DIR_ARG, "=", "<path>"), "Specify the downloads root directory");
table.format("", "(default: " + format_environment_variable("VCPKG_DOWNLOADS") + ')');
table.format(opt(VCPKG_ROOT_DIR_ARG, " ", "<path>"), "Specify the vcpkg root directory");
table.format(opt(VCPKG_ROOT_DIR_ARG, "=", "<path>"), "Specify the vcpkg root directory");
table.format("", "(default: " + format_environment_variable("VCPKG_ROOT") + ')');
table.format(opt(BUILDTREES_ROOT_DIR_ARG, "=", "<path>"),
"(Experimental) Specify the buildtrees root directory");
Expand Down