[build rules] Switch codegen rules to run codegen directly, with no shell

xls/build_rules/xls_codegen_rules.bzl

@@ -289,14 +289,20 @@ def xls_ir_verilog_impl(ctx, src, conv_info):
     # parse arguments
 
     is_args_valid(codegen_args, _CODEGEN_FLAGS + _SCHEDULING_FLAGS)
-    codegen_str_args = args_to_string(codegen_args, _CODEGEN_FLAGS + _SCHEDULING_FLAGS)
     uses_combinational_generator = _is_combinational_generator(codegen_args)
-    final_args = codegen_str_args
+
+    args = ctx.actions.args()
+    args.add(src.ir_file)
+    for flag, value in codegen_args.items():
+        # Need to handle that some flag values are things like 'true' and
+        # 'false' that need to be handled carefully.
+        args.add("--{}={}".format(flag, value))
+
     if (ctx.file.codegen_options_proto == None and conv_info.ir_interface != None):
         # Mixing proto options and normal ones is not supported. If proto
         # options are being used the user will need to have put the interface
         # proto in manually.
-        final_args += " --ir_interface_proto={}".format(conv_info.ir_interface.path)
+        args.add("--ir_interface_proto={}".format(conv_info.ir_interface.path))
         runfiles_list.append(conv_info.ir_interface)
 
     uses_fdo = _uses_fdo(codegen_args)
@@ -319,7 +325,7 @@ def xls_ir_verilog_impl(ctx, src, conv_info):
     )
     verilog_line_map_file = ctx.actions.declare_file(verilog_line_map_filename)
     my_generated_files.append(verilog_line_map_file)
-    final_args += " --output_verilog_line_map_path={}".format(verilog_line_map_file.path)
+    args.add("--output_verilog_line_map_path={}".format(verilog_line_map_file.path))
 
     schedule_file = None
     if not uses_combinational_generator:
@@ -332,7 +338,7 @@ def xls_ir_verilog_impl(ctx, src, conv_info):
 
         schedule_file = ctx.actions.declare_file(schedule_filename)
         my_generated_files.append(schedule_file)
-        final_args += " --output_schedule_path={}".format(schedule_file.path)
+        args.add("--output_schedule_path={}".format(schedule_file.path))
 
     verilog_file = ctx.actions.declare_file(verilog_filename)
     module_sig_filename = get_output_filename_value(
@@ -342,40 +348,38 @@ def xls_ir_verilog_impl(ctx, src, conv_info):
     )
     module_sig_file = ctx.actions.declare_file(module_sig_filename)
     my_generated_files += [verilog_file, module_sig_file]
-    final_args += " --output_verilog_path={}".format(verilog_file.path)
-    final_args += " --output_signature_path={}".format(module_sig_file.path)
+    args.add("--output_verilog_path={}".format(verilog_file.path))
+    args.add("--output_signature_path={}".format(module_sig_file.path))
     schedule_ir_filename = get_output_filename_value(
         ctx,
         "schedule_ir_file",
         verilog_basename + _SCHEDULE_IR_FILE_EXTENSION,
     )
     schedule_ir_file = ctx.actions.declare_file(schedule_ir_filename)
-    final_args += " --output_schedule_ir_path={}".format(schedule_ir_file.path)
+    args.add("--output_schedule_ir_path={}".format(schedule_ir_file.path))
     my_generated_files.append(schedule_ir_file)
     block_ir_filename = get_output_filename_value(
         ctx,
         "block_ir_file",
         verilog_basename + _BLOCK_IR_FILE_EXTENSION,
     )
     block_ir_file = ctx.actions.declare_file(block_ir_filename)
-    final_args += " --output_block_ir_path={}".format(block_ir_file.path)
+    args.add("--output_block_ir_path={}".format(block_ir_file.path))
     my_generated_files.append(block_ir_file)
 
     # Get runfiles
     codegen_tool_runfiles = ctx.attr._xls_codegen_tool[DefaultInfo].default_runfiles
 
     if ctx.file.codegen_options_proto:
-        final_args += " --codegen_options_proto={}".format(ctx.file.codegen_options_proto.path)
+        args.add("--codegen_options_proto={}".format(ctx.file.codegen_options_proto.path))
         runfiles_list.append(ctx.file.codegen_options_proto)
 
     if ctx.file.scheduling_options_proto:
-        final_args += " --scheduling_options_proto={}".format(ctx.file.scheduling_options_proto.path)
+        args.add("--scheduling_options_proto={}".format(ctx.file.scheduling_options_proto.path))
         runfiles_list.append(ctx.file.scheduling_options_proto)
 
     runfiles = get_runfiles_for_xls(ctx, [codegen_tool_runfiles], runfiles_list)
 
-    tools = [codegen_tool]
-
     codegen_log_filename = get_output_filename_value(
         ctx,
         "codegen_log_file",
@@ -391,30 +395,23 @@ def xls_ir_verilog_impl(ctx, src, conv_info):
     )
     config_textproto_file = ctx.actions.declare_file(codegen_config_textproto_file)
     my_generated_files.append(config_textproto_file)
-    final_args += " --codegen_options_used_textproto_file={}".format(
-        config_textproto_file.path,
-    )
+    args.add("--codegen_options_used_textproto_file={}".format(config_textproto_file.path))
     schedule_config_textproto_file = get_output_filename_value(
         ctx,
         "scheduling_options_used_textproto_file",
         verilog_basename + _SCHEDULING_OPTIONS_USED_TEXTPROTO_FILE_EXTENSION,
     )
     sched_config_textproto_file = ctx.actions.declare_file(schedule_config_textproto_file)
     my_generated_files.append(sched_config_textproto_file)
-    final_args += " --scheduling_options_used_textproto_file={}".format(
-        sched_config_textproto_file.path,
-    )
+    args.add("--scheduling_options_used_textproto_file={}".format(sched_config_textproto_file.path))
+
+    args.add("--alsologto", log_file)
 
-    ctx.actions.run_shell(
+    ctx.actions.run(
         outputs = my_generated_files,
-        tools = tools,
+        executable = codegen_tool,
         inputs = runfiles.files,
-        command = "set -o pipefail; {} {} {} 2>&1 | tee {}".format(
-            codegen_tool.path,
-            src.ir_file.path,
-            final_args,
-            log_file.path,
-        ),
+        arguments = [args],
         mnemonic = "GenerateVerilog",
         progress_message = "Compiling %s" % verilog_file.short_path,
         toolchain = None,

xls/tools/BUILD

@@ -382,11 +382,25 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "file_stderr_log_sink",
+    srcs = ["file_stderr_log_sink.cc"],
+    hdrs = ["file_stderr_log_sink.h"],
+    deps = [
+        "//xls/common/file:filesystem",
+        "@com_google_absl//absl/log:check",
+        "@com_google_absl//absl/log:globals",
+        "@com_google_absl//absl/log:log_entry",
+        "@com_google_absl//absl/log:log_sink",
+    ],
+)
+
 cc_binary(
     name = "opt_main",
     srcs = ["opt_main.cc"],
     visibility = ["//xls:xls_users"],
     deps = [
+        ":file_stderr_log_sink",
         ":opt",
         "//xls/common:exit_status",
         "//xls/common:init_xls",
@@ -686,6 +700,7 @@ cc_binary(
         ":codegen",
         ":codegen_flags",
         ":codegen_flags_cc_proto",
+        ":file_stderr_log_sink",
         ":scheduling_options_flags",
         ":scheduling_options_flags_cc_proto",
         "//xls/codegen:module_signature",
@@ -700,9 +715,12 @@ cc_binary(
         "//xls/ir:verifier",
         "//xls/scheduling:pipeline_schedule_cc_proto",
         "//xls/scheduling:scheduling_options",
+        "@com_google_absl//absl/cleanup",
         "@com_google_absl//absl/flags:flag",
         "@com_google_absl//absl/log",
         "@com_google_absl//absl/log:check",
+        "@com_google_absl//absl/log:log_sink",
+        "@com_google_absl//absl/log:log_sink_registry",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings:str_format",
     ],

xls/tools/codegen_main.cc

@@ -20,9 +20,12 @@
 #include <string_view>
 #include <vector>
 
+#include "absl/cleanup/cleanup.h"
 #include "absl/flags/flag.h"
 #include "absl/log/check.h"
 #include "absl/log/log.h"
+#include "absl/log/log_sink.h"
+#include "absl/log/log_sink_registry.h"
 #include "absl/status/status.h"
 #include "absl/strings/str_format.h"
 #include "xls/codegen/module_signature.h"
@@ -40,6 +43,7 @@
 #include "xls/tools/codegen.h"
 #include "xls/tools/codegen_flags.h"
 #include "xls/tools/codegen_flags.pb.h"
+#include "xls/tools/file_stderr_log_sink.h"
 #include "xls/tools/scheduling_options_flags.h"
 #include "xls/tools/scheduling_options_flags.pb.h"
 
@@ -58,6 +62,9 @@ Emit a feed-forward pipelined module:
        IR_FILE
 )";
 
+ABSL_FLAG(std::optional<std::string>, alsologto, std::nullopt,
+          "Path to write logs to, in addition to stderr.");
+
 namespace xls {
 namespace {
 
@@ -66,6 +73,19 @@ absl::Status RealMain(std::string_view ir_path) {
   if (ir_path == "-") {
     ir_path = "/dev/stdin";
   }
+
+  std::optional<std::string> alsologto = absl::GetFlag(FLAGS_alsologto);
+  std::unique_ptr<absl::LogSink> log_file_sink;
+  if (alsologto.has_value()) {
+    log_file_sink = std::make_unique<FileStderrLogSink>(*alsologto);
+    absl::AddLogSink(log_file_sink.get());
+  }
+  absl::Cleanup log_file_sink_cleanup = [&log_file_sink] {
+    if (log_file_sink) {
+      absl::RemoveLogSink(log_file_sink.get());
+    }
+  };
+
   XLS_ASSIGN_OR_RETURN(std::string ir_contents, GetFileContents(ir_path));
   XLS_ASSIGN_OR_RETURN(std::unique_ptr<Package> p,
                        Parser::ParsePackage(ir_contents, ir_path));

xls/tools/file_stderr_log_sink.cc

@@ -0,0 +1,45 @@
+// Copyright 2025 The XLS Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "xls/tools/file_stderr_log_sink.h"
+
+#include <filesystem>  // NOLINT
+#include <utility>
+
+#include "absl/log/check.h"
+#include "absl/log/globals.h"
+#include "absl/log/log_entry.h"
+#include "xls/common/file/filesystem.h"
+
+namespace xls {
+
+FileStderrLogSink::FileStderrLogSink(std::filesystem::path path)
+    : path_(std::move(path)) {
+  CHECK_OK(SetFileContents(path_, ""));
+}
+
+void FileStderrLogSink::Send(const absl::LogEntry& entry) {
+  if (entry.log_severity() < absl::StderrThreshold()) {
+    return;
+  }
+
+  if (!entry.stacktrace().empty()) {
+    CHECK_OK(AppendStringToFile(path_, entry.stacktrace()));
+  } else {
+    CHECK_OK(AppendStringToFile(path_,
+                                entry.text_message_with_prefix_and_newline()));
+  }
+}
+
+}  // namespace xls

xls/tools/file_stderr_log_sink.h

@@ -0,0 +1,39 @@
+// Copyright 2025 The XLS Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef XLS_TOOLS_FILE_LOG_SINK_H_
+#define XLS_TOOLS_FILE_LOG_SINK_H_
+
+#include <filesystem>  // NOLINT
+
+#include "absl/log/log_entry.h"
+#include "absl/log/log_sink.h"
+
+namespace xls {
+
+class FileStderrLogSink final : public absl::LogSink {
+ public:
+  explicit FileStderrLogSink(std::filesystem::path path);
+
+  ~FileStderrLogSink() override = default;
+
+  void Send(const absl::LogEntry& entry) override;
+
+ private:
+  const std::filesystem::path path_;
+};
+
+}  // namespace xls
+
+#endif  // XLS_TOOLS_FILE_LOG_SINK_H_

xls/tools/opt_main.cc

@@ -52,6 +52,7 @@
 #include "xls/passes/pass_base.h"
 #include "xls/passes/pass_metrics.pb.h"
 #include "xls/passes/pass_pipeline.pb.h"
+#include "xls/tools/file_stderr_log_sink.h"
 #include "xls/tools/opt.h"
 
 static constexpr std::string_view kUsage = R"(
@@ -150,32 +151,6 @@ ABSL_FLAG(bool, debug_optimizations, false,
 namespace xls::tools {
 namespace {
 
-class FileStderrLogSink final : public absl::LogSink {
- public:
-  explicit FileStderrLogSink(std::filesystem::path path)
-      : path_(std::move(path)) {
-    CHECK_OK(SetFileContents(path_, ""));
-  }
-
-  ~FileStderrLogSink() override = default;
-
-  void Send(const absl::LogEntry& entry) override {
-    if (entry.log_severity() < absl::StderrThreshold()) {
-      return;
-    }
-
-    if (!entry.stacktrace().empty()) {
-      CHECK_OK(AppendStringToFile(path_, entry.stacktrace()));
-    } else {
-      CHECK_OK(AppendStringToFile(
-          path_, entry.text_message_with_prefix_and_newline()));
-    }
-  }
-
- private:
-  const std::filesystem::path path_;
-};
-
 template <typename T>
   requires(std::is_integral_v<T>)
 std::optional<T> NegativeIsNullopt(std::optional<T> v) {