Skip to content

Always use a param file when calling into rustc #1076

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions rust/private/rustc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,8 @@ def construct_arguments(
force_all_deps_direct = False,
force_link = False,
stamp = False,
remap_path_prefix = "."):
remap_path_prefix = ".",
use_param_file_for_rustc_args = True):
"""Builds an Args object containing common rustc flags

Args:
Expand All @@ -500,6 +501,7 @@ def construct_arguments(
stamp (bool, optional): Whether or not workspace status stamping is enabled. For more details see
https://docs.bazel.build/versions/main/user-manual.html#flag--stamp
remap_path_prefix (str, optional): A value used to remap `${pwd}` to. If set to a falsey value, no prefix will be set.
use_param_file_for_rustc_args(bool, optional): Whether Bazel should use a param file to pass the arguments to rustc.

Returns:
tuple: A tuple of the following items
Expand Down Expand Up @@ -574,8 +576,11 @@ def construct_arguments(

# Rustc arguments
rustc_flags = ctx.actions.args()
rustc_flags.set_param_file_format("multiline")
rustc_flags.use_param_file("@%s", use_always = False)

if use_param_file_for_rustc_args:
rustc_flags.set_param_file_format("multiline")
rustc_flags.use_param_file("@%s", use_always = True)

rustc_flags.add(crate_info.root)
rustc_flags.add("--crate-name=" + crate_info.name)
rustc_flags.add("--crate-type=" + crate_info.type)
Expand Down
5 changes: 4 additions & 1 deletion rust/private/rustdoc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ def rustdoc_compile_action(
crate_info,
output = None,
rustdoc_flags = [],
is_test = False):
is_test = False,
use_param_file_for_rustc_args = True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a small unittest ensuring that when this flag is True prama files prefixed with @ are found in argv of the action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried doing that but it looks like Action.argv returns the actual arguments for the action before they are turned into a file, rather than the arguments passed to the executable...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's kinda crazy. I feel like this is a bug in Bazel... Is there a way we can at least test two different actions with and without this flag?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose not. That's the point of the "always" in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup this is an unfortunate and a known limitation of unittesting args. You can access the original args value from the unittest (https://docs.bazel.build/versions/main/skylark/lib/Action.html#args), but you cannot do anything with it other than setting it as args into some other action. So the only way we could test this is by writing all these args into a file and registering an action that reads the file and makes assertions. In other words, you cannot use unittest, you need an integration test.

"""Create a struct of information needed for a `rustdoc` compile action based on crate passed to the rustdoc rule.

Args:
Expand All @@ -59,6 +60,7 @@ def rustdoc_compile_action(
output (File, optional): An optional output a `rustdoc` action is intended to produce.
rustdoc_flags (list, optional): A list of `rustdoc` specific flags.
is_test (bool, optional): If True, the action will be configured for `rust_doc_test` targets
use_param_file_for_rustc_args(bool, optional): Whether Bazel should use a param file to pass the arguments to rustc.

Returns:
struct: A struct of some `ctx.actions.run` arguments.
Expand Down Expand Up @@ -118,6 +120,7 @@ def rustdoc_compile_action(
emit = [],
remap_path_prefix = None,
force_link = True,
use_param_file_for_rustc_args = use_param_file_for_rustc_args,
)

return struct(
Expand Down
3 changes: 3 additions & 0 deletions rust/private/rustdoc_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ def _rust_doc_test_impl(ctx):
crate_info = crate_info,
rustdoc_flags = rustdoc_flags,
is_test = True,
# The arguments are written to a script file that is used to run the
# tests and the args point to a param file outside of the `runfiles`.
use_param_file_for_rustc_args = False,
)

tools = action.tools + [ctx.executable._process_wrapper]
Expand Down
98 changes: 70 additions & 28 deletions util/process_wrapper/process_wrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,41 @@
#include "util/process_wrapper/system.h"
#include "util/process_wrapper/utils.h"

using CharType = process_wrapper::System::StrType::value_type;
using namespace process_wrapper;
using CharType = System::StrType::value_type;

static int process_param_file(System::StrType& path,
System::Arguments& collected_args,
const std::vector<Subst>& subst_mappings) {
System::StrVecType args;
if (!ReadFileToVec(path, args)) {
std::cerr << "Cannot read param file " << ToUtf8(path) << "\n";
return -1;
}
for (System::StrType& arg : args) {
// Expand tokens into each argument (since some paths might embed `${pwd}`
// that we need to resolve from the subsitution mappings).
ReplaceTokens(arg, subst_mappings);

// If this is a param file, expand it into the top-level arguments by
// recursing into its content.
if (arg.size() > 0 && arg.substr(0, 1) == PW_SYS_STR("@")) {
System::StrType path = arg.substr(1);
int res = process_param_file(path, collected_args, subst_mappings);
if (res != 0) {
return res;
}
} else {
collected_args.push_back(arg);
}
}
return 0;
}

// Simple process wrapper allowing us to not depend on the shell to run a
// process to perform basic operations like capturing the output or having
// the $pwd used in command line arguments or environment variables
int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
using namespace process_wrapper;

System::EnvironmentBlock environment_block;
// Taking all environment variables from the current process
// and sending them down to the child process
Expand All @@ -38,8 +65,6 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {

System::EnvironmentBlock environment_file_block;

using Subst = std::pair<System::StrType, System::StrType>;

System::StrType exec_path;
std::vector<Subst> subst_mappings;
std::vector<Subst> stamp_mappings;
Expand All @@ -53,7 +78,7 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
System::Arguments file_arguments;

// Processing current process argument list until -- is encountered
// everthing after gets sent down to the child process
// everything after gets sent down to the child process
for (int i = 1; i < argc; ++i) {
System::StrType arg = argv[i];
if (++i == argc) {
Expand All @@ -79,7 +104,8 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
if (value == PW_SYS_STR("${pwd}")) {
value = System::GetWorkingDirectory();
}
subst_mappings.push_back({std::move(key), std::move(value)});
subst_mappings.push_back({PW_SYS_STR("${") + key + PW_SYS_STR("}"),
std::move(value)});
} else if (arg == PW_SYS_STR("--volatile-status-file")) {
if (!volatile_status_file.empty()) {
std::cerr << "process wrapper error: \"--volatile-status-file\" can "
Expand All @@ -90,11 +116,11 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
return -1;
}
} else if (arg == PW_SYS_STR("--env-file")) {
if (!ReadFileToArray(argv[i], environment_file_block)) {
if (!ReadFileToVec(argv[i], environment_file_block)) {
return -1;
}
} else if (arg == PW_SYS_STR("--arg-file")) {
if (!ReadFileToArray(argv[i], file_arguments)) {
if (!ReadFileToVec(argv[i], file_arguments)) {
return -1;
}
} else if (arg == PW_SYS_STR("--touch-file")) {
Expand Down Expand Up @@ -143,7 +169,7 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
for (++i; i < argc; ++i) {
arguments.push_back(argv[i]);
}
// after we consume all arguments we append the files arguments
// After we have consumed all arguments add the files arguments the them.
for (const System::StrType& file_arg : file_arguments) {
arguments.push_back(file_arg);
}
Expand All @@ -154,13 +180,39 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
}
}

// Go through the arguments and make sure that we process param files so that
// we can flatten potential recursive param files within them.
for (System::StrType& arg : arguments) {
if (arg.size() > 0 && arg.substr(0, 1) == PW_SYS_STR("@")) {
// This is a param file, process it separately.
System::StrType path = arg.substr(1);
System::Arguments processed_args;
if (process_param_file(path, processed_args, subst_mappings) != 0) {
std::cerr << "Failure to process arguments list\n";
return -1;
}

// Create a new param file so that we don't need to edit the original one
// that Bazel created.
System::StrType new_path = path + PW_SYS_STR(".expanded");
if (!WriteVecToFile(new_path, processed_args)) {
std::cerr << "Cannot write params to file " << ToUtf8(new_path) << "\n";
return -1;
}

// Replace the params file arg with the new one.
arg = PW_SYS_STR("@") + new_path;
} else {
// Expand tokens into each argument (since some paths might embed `${pwd}`
// that we need to resolve from the subsitution mappings).
ReplaceTokens(arg, subst_mappings);
}
}

// Stamp any format string in an environment variable block
for (const Subst& stamp : stamp_mappings) {
System::StrType token = PW_SYS_STR("{");
token += stamp.first;
token.push_back('}');
if (stamp_mappings.size()) {
for (System::StrType& env : environment_file_block) {
ReplaceToken(env, token, stamp.second);
ReplaceTokens(env, stamp_mappings);
}
}

Expand All @@ -171,19 +223,9 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) {
environment_file_block.begin(),
environment_file_block.end());

if (subst_mappings.size()) {
for (const Subst& subst : subst_mappings) {
System::StrType token = PW_SYS_STR("${");
token += subst.first;
token.push_back('}');
for (System::StrType& arg : arguments) {
ReplaceToken(arg, token, subst.second);
}

for (System::StrType& env : environment_block) {
ReplaceToken(env, token, subst.second);
}
}
// Expand tokens in environment
for (System::StrType& env : environment_block) {
ReplaceTokens(env, subst_mappings);
}

// Have the last values added take precedence over the first.
Expand Down
33 changes: 24 additions & 9 deletions util/process_wrapper/utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,17 @@ std::string ToUtf8(const System::StrType& string) {
#endif // defined(PW_WIN_UNICODE)
}

void ReplaceToken(System::StrType& str, const System::StrType& token,
const System::StrType& replacement) {
std::size_t pos = str.find(token);
if (pos != std::string::npos) {
str.replace(pos, token.size(), replacement);
void ReplaceTokens(System::StrType& str, const std::vector<Subst>& substs) {
for (const Subst& subst : substs) {
std::size_t pos = str.find(subst.first);
if (pos != std::string::npos) {
str.replace(pos, subst.first.size(), subst.second);
}
}
}

bool ReadFileToArray(const System::StrType& file_path,
System::StrVecType& vec) {
bool ReadFileToVec(const System::StrType& file_path,
System::StrVecType& vec) {
std::ifstream file(file_path);
if (file.fail()) {
std::cerr << "process wrapper error: failed to open file: "
Expand Down Expand Up @@ -101,12 +102,25 @@ bool ReadFileToArray(const System::StrType& file_path,
return true;
}

bool WriteVecToFile(const System::StrType& file_path, const System::StrVecType& vec) {
std::ofstream file(file_path, std::ofstream::out | std::ofstream::trunc);
if (file.fail()) {
std::cerr << "process wrapper error: failed to open file: "
<< ToUtf8(file_path) << '\n';
return false;
}
for (const System::StrType& line : vec) {
file << ToUtf8(line) << std::endl;
}
return true;
}

bool ReadStampStatusToArray(
const System::StrType& stamp_path,
std::vector<std::pair<System::StrType, System::StrType>>& vec) {
// Read each line of the stamp file and split on the first space
System::StrVecType stamp_block;
if (!ReadFileToArray(stamp_path, stamp_block)) {
if (!ReadFileToVec(stamp_path, stamp_block)) {
return false;
}

Expand All @@ -118,7 +132,8 @@ bool ReadStampStatusToArray(
<< ToUtf8(stamp_block[i]) << "\".\n";
return false;
}
System::StrType key = stamp_block[i].substr(0, space_pos);
System::StrType key =
PW_SYS_STR("{") + stamp_block[i].substr(0, space_pos) + PW_SYS_STR("}");
System::StrType value =
stamp_block[i].substr(space_pos + 1, stamp_block[i].size());
vec.push_back({std::move(key), std::move(value)});
Expand Down
12 changes: 8 additions & 4 deletions util/process_wrapper/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,20 @@

namespace process_wrapper {

using Subst = std::pair<System::StrType, System::StrType>;

// Converts to and frin the system string format
System::StrType FromUtf8(const std::string& string);
std::string ToUtf8(const System::StrType& string);

// Replaces a token in str by replacement
void ReplaceToken(System::StrType& str, const System::StrType& token,
const System::StrType& replacement);
// Replaces tokens in str by replacement
void ReplaceTokens(System::StrType& str, const std::vector<Subst>& substs);

// Reads a file in text mode and feeds each line to item in the vec output
bool ReadFileToArray(const System::StrType& file_path, System::StrVecType& vec);
bool ReadFileToVec(const System::StrType& file_path, System::StrVecType& vec);

// Write the content of an array file as lines in a file
bool WriteVecToFile(const System::StrType& file_path, const System::StrVecType& vec);

// Reads a workspace status stamp file to an array of key value pairs
bool ReadStampStatusToArray(
Expand Down