Avoid leaking doskey macros from internal vcvarsall runs. #1295
Conversation
include/vcpkg/base/system.process.h
Outdated
| }; | ||
|
|
||
| extern const WorkingDirectory default_working_directory; | ||
| extern const Environment default_environment; | ||
| struct RedirectedProcessLaunchSettings : ProcessLaunchSettings |
There was a problem hiding this comment.
I'm concerned about inheriting here due to implicit conversion. E.g. cmd_execute(redirected) will do the wrong thing and it isn't terribly obvious.
Alternatively, cmd_execute() should take a different inherited type UnredirectedProcessLaunchSettings that doesn't have additional fields over the base.
There was a problem hiding this comment.
E.g. cmd_execute(redirected) will do the wrong thing and it isn't terribly obvious.
It isn't clear to me that it does the wrong thing. The only difference between these structs is that the settings in the redirected block are about redirection. The only reason they aren't the same struct is so that cmd_execute isn't claiming to respect settings it does not.
However, I also don't see any reason for it to ever convert so I changed it by duplicating the members.
include/vcpkg/base/system.process.h
Outdated
| ProcessLaunchSettings(const Command& cmd) : cmd(cmd) { } | ||
| ProcessLaunchSettings(Command&& cmd) : cmd(cmd) { } | ||
|
|
||
| ProcessLaunchSettings& string_arg(StringView s) & |
There was a problem hiding this comment.
It looks like this fluent style isn't needed much -- most callers already (or could with small tweaks) use it to construct the Command before constructing the ProcessLaunchSettings.
I could see this making a lot of sense if there were common APIs which wanted to return an XYZProcessLaunchSettings that we'd like to chain off of.
src/vcpkg/base/downloads.cpp
Outdated
| int code = 0; | ||
| auto res = cmd_execute_and_stream_lines(cmd, [&code](StringView line) { | ||
| auto res = cmd_execute_and_stream_lines(settings, [&code](StringView line) { |
There was a problem hiding this comment.
Why does every call site need to change, compared to having an overload that takes a Command and uses the other defaults from RedirectedProcessLaunchSettings?
| { | ||
| case Encoding::Utf8: | ||
| raw_cb = [&](char* buf, size_t bytes_read) { | ||
| std::replace(buf, buf + bytes_read, '\0', '?'); | ||
| data_cb(StringView{buf, bytes_read}); | ||
| if (settings.echo_in_debug == EchoInDebug::Show && Debug::g_debugging) |
There was a problem hiding this comment.
Should we echo to debug before processing? If processing takes a long time or crashes, that would ensure the debug output is available.
There was a problem hiding this comment.
That makes sense to me. Note though that this is the same order in the existing product:
vcpkg-tool/src/vcpkg/base/system.process.cpp
Lines 1783 to 1786 in 2f25fc8
src/vcpkg/binarycaching.cpp
Outdated
| @@ -573,7 +576,7 @@ namespace | |||
| }); | |||
| } | |||
|
|
|||
| return cmd_execute_and_capture_output(cmdline).then([&](ExitCodeAndOutput&& res) -> ExpectedL<Unit> { | |||
| return cmd_execute_and_capture_output(settings).then([&](ExitCodeAndOutput&& res) -> ExpectedL<Unit> { | |||
| if (Debug::g_debugging) | |||
There was a problem hiding this comment.
Is this replaced by your "echo in debug" parameter?
src/vcpkg/binarycaching.cpp
Outdated
| @@ -1078,7 +1081,7 @@ namespace | |||
| return msg::format(msgRestoredPackagesFromGCS, msg::count = count, msg::elapsed = ElapsedTime(elapsed)); | |||
| } | |||
|
|
|||
| Command command() const { return m_tool; } | |||
| RedirectedProcessLaunchSettings command() const { return RedirectedProcessLaunchSettings{Command{m_tool}}; } | |||
There was a problem hiding this comment.
I think this should continue to return a Command. This function doesn't need to prescribe any of the other parameters of RedirectedProcessLaunchSettings like "echo in debug" or "working directory". If there are things that this should prescribe, that sounds like there's an intermediate type that should be returned instead.
|
|
||
| const auto& abi_info = action.abi_info.value_or_exit(VCPKG_LINE_INFO); | ||
| auto env = paths.get_action_env(*abi_info.pre_build_info, abi_info.toolset.value_or_exit(VCPKG_LINE_INFO)); | ||
| auto& env = settings.environment.emplace( |
There was a problem hiding this comment.
Why emplace over assign?
src/vcpkg/commands.build.cpp
Outdated
| const auto& env = paths.get_action_env(pre_build_info, toolset); | ||
| RedirectedProcessLaunchSettings settings; | ||
| settings.cmd = vcpkg::make_cmake_cmd(paths, paths.ports_cmake, std::move(cmake_args)); | ||
| settings.environment = paths.get_action_env(pre_build_info, toolset); |
There was a problem hiding this comment.
Why assign over emplace?
| @@ -127,31 +127,31 @@ namespace vcpkg | |||
| } | |||
| } | |||
|
|
|||
| auto env = get_modified_clean_environment(extra_env); | |||
| ProcessLaunchSettings settings; | |||
| auto& env = settings.environment.emplace(get_modified_clean_environment(extra_env)); | |||
| if (!build_env_cmd.empty()) | |||
| { | |||
| #if defined(_WIN32) | |||
| env = cmd_execute_and_capture_environment(build_env_cmd, env); | |||
There was a problem hiding this comment.
Does this need adjusting to avoid doskey macros?
There was a problem hiding this comment.
I don't think this one is fixable, since the intent here is for the user to be able to use the terminal inside, that means it has to be in their terminal window.
There was a problem hiding this comment.
Ah, this caller does not, because cmd_execute_and_capture_environment itself is what passes CreateNewConsole::Yes.
src/vcpkg/vcpkgpaths.cpp
Outdated
|
|
||
| auto env = default_environment; | ||
| auto& env = git_archive.environment.emplace(); |
There was a problem hiding this comment.
Is default_environment the same as a default constructed environment?
There was a problem hiding this comment.
vcpkg-tool/src/vcpkg/base/system.process.cpp
Lines 669 to 670 in 2f25fc8
Yes
# Conflicts: # src/vcpkg/base/system.process.cpp # src/vcpkg/commands.build.cpp # src/vcpkg/commands.export.cpp # src/vcpkg/commands.integrate.cpp # src/vcpkg/configure-environment.cpp # src/vcpkg/vcpkgpaths.cpp
| @@ -127,34 +127,26 @@ namespace vcpkg | |||
| } | |||
| } | |||
|
|
|||
| auto env = get_modified_clean_environment(extra_env); | |||
| #if defined(_WIN32) | |||
There was a problem hiding this comment.
Note that the old code on non-Win32 was:
auto env = get_modified_clean_environment(extra_env);
if (!build_env_cmd.empty())
{
Checks::msg_exit_with_message(VCPKG_LINE_INFO, msgEnvPlatformNotSupported); // <-- !!!
}
Command cmd("");
Checks::msg_exit_with_message(VCPKG_LINE_INFO, msgEnvPlatformNotSupported); // <-- !!!
if (!options.command_arguments.empty())
{
cmd.string_arg("/c").raw_arg(options.command_arguments[0]);
}
auto rc = cmd_execute(cmd, default_working_directory, env);
Checks::exit_with_code(VCPKG_LINE_INFO, rc.value_or_exit(VCPKG_LINE_INFO));| .string_arg(".vcpkg-root"), | ||
| settings), | ||
| Tools::GIT) | ||
| .value_or_exit(VCPKG_LINE_INFO); |
There was a problem hiding this comment.
I believe this is a behavior change from previous -- if the git command failed, we would silently attempt to continue.
Agreed with the change, but highlighting it in case we see issues later.
…ted as 1 argument rather than many.
…ted as 1 argument rather than many.
This is a resolution for microsoft/vcpkg#32641 .
First, I transform all the process launch things to use a struct instead, because this needs to add yet another parameter and if I had to touch all callers to think about this parameter anyway, it made sense to just do the struct transformation now.
Second, this adds a new field to that struct
CreateNewConsole, which if true runs things in a new console, which insulates us fromdoskey. Notably, there is a performance cost for creating new consoles, so we only want to do it when we think it is likely to matter, like here.Below is a demonstration of the bug, and that the bug is fixed after this change:
== broken
== fixed