-
Notifications
You must be signed in to change notification settings - Fork 272
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
Avoid leaking doskey macros from internal vcvarsall runs. #1295
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publishing a partial review through commands.export.cpp
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Strings::append(output, sv); | |
if (echo_in_debug == EchoInDebug::Show && Debug::g_debugging) | |
{ | |
msg::write_unlocalized_text_to_stdout(Color::none, sv); |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this replaced by your "echo in debug" parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be yes
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need adjusting to avoid doskey macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this caller does not, because cmd_execute_and_capture_environment
itself is what passes CreateNewConsole::Yes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished review.
src/vcpkg/vcpkgpaths.cpp
Outdated
|
||
auto env = default_environment; | ||
auto& env = git_archive.environment.emplace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is default_environment
the same as a default constructed environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vcpkg-tool/src/vcpkg/base/system.process.cpp
Lines 669 to 670 in 2f25fc8
const WorkingDirectory default_working_directory; | |
const Environment default_environment; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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