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
[vcpkg] Rewriting CmdLineBuilder (2/n) #15627
Conversation
I would like, and I think the team would like generally, to switch to using stuff like `posix_spawn`, as opposed to the existing use of `system` and `popen`. This requires a pretty large change to how we use CmdLineBuilder. The first change we have to make is that the execute functions _cannot_ take a StringView anymore. This PR makes that change.
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.
Woooooooooo
toolsrc/src/vcpkg/commands.edit.cpp
Outdated
cmd_line = System::CmdLineBuilder("cmd").string_arg("/c").string_arg(cmd_line).raw_arg("<NUL"); | ||
System::cmd_execute_background(cmd_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.
cmd_line = System::CmdLineBuilder("cmd").string_arg("/c").string_arg(cmd_line).raw_arg("<NUL"); | |
System::cmd_execute_background(cmd_line); | |
System::cmd_execute_background(System::CmdLineBuilder("cmd").string_arg("/c").string_arg(cmd_line).raw_arg("<NUL")); |
This also enables cmd_line
to be made const again above.
System::CmdLineBuilder cmd("cmd"); | ||
if (!args.command_arguments.empty()) | ||
{ | ||
cmd.string_arg("/c").raw_arg(args.command_arguments.at(0)); |
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 know this is just preserving existing semantics, but I think this would probably be more correct as .string_arg()
.string_arg(Strings::concat("--git-dir=", fs::u8string(dot_git_directory))) | ||
.string_arg(Strings::concat("--work-tree=", fs::u8string(working_directory))); | ||
const std::string full_cmd = Strings::concat(std::move(builder).extract(), " ", cmd); | ||
auto full_cmd = System::CmdLineBuilder(git_exe) |
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.
This entire function seems really suspect with the new approach; all uses of this should probably be replaced with the make-a-git-CmdLineBuilder-function.
5cbc281
to
ec6621e
Compare
fc7f769
to
0c3f859
Compare
0c3f859
to
b84e75e
Compare
* [vcpkg] Rewriting CmdLineBuilder (2/n) I would like, and I think the team would like generally, to switch to using stuff like `posix_spawn`, as opposed to the existing use of `system` and `popen`. This requires a pretty large change to how we use CmdLineBuilder. The first change we have to make is that the execute functions _cannot_ take a StringView anymore. This PR makes that change.
I would like, and I think the team would like generally,
to switch to using stuff like
posix_spawn
, as opposed to theexisting use of
system
andpopen
.This requires a pretty large change to how we use CmdLineBuilder.
The first change we have to make is that the execute functions
cannot take a StringView anymore.
This PR makes that change.