Stream: git-wasmtime

Topic: wasmtime / issue #9553 Avoid printing full file paths in ...

Wasmtime GitHub notifications bot (Nov 05 2024 at 08:43):

nmattia opened issue #9553:

Feature

It would be great to avoid printing full paths of source files in the generated code.

Benefit

This would allow for better or more correct caching in third party build systems like Bazel or Nix.

Implementation

1. The naive solution is to remove references to the source files:

diff --git a/cranelift/codegen/meta/src/srcgen.rs b/cranelift/codegen/meta/src/srcgen.rs
index d3c321e5b..5b94ddd19 100644
--- a/cranelift/codegen/meta/src/srcgen.rs
+++ b/cranelift/codegen/meta/src/srcgen.rs
@@ -94,7 +94,6 @@ impl Formatter {
         directory: &std::path::Path,
     ) -> Result<(), error::Error> {
         let path = directory.join(&filename);
-        eprintln!("Writing generated file: {}", path.display());
         let mut f = fs::File::create(path)?;

         for l in self.lines.iter().map(|l| l.as_bytes()) {
diff --git a/cranelift/isle/isle/src/codegen.rs b/cranelift/isle/isle/src/codegen.rs
index 158285832..d292e43c0 100644
--- a/cranelift/isle/isle/src/codegen.rs
+++ b/cranelift/isle/isle/src/codegen.rs
@@ -127,9 +127,6 @@ impl<'a> Codegen<'a> {
             "// Generated automatically from the instruction-selection DSL code in:",
         )
         .unwrap();
-        for file in &self.files.file_names {
-            writeln!(code, "// - {file}").unwrap();
-        }

         if !options.exclude_global_allow_pragmas {
             writeln!(
@@ -641,12 +638,11 @@ impl<L: Length, C> Length for ContextIterWrapper<L, C> {{
                             stack.push((Self::validate_block(ret_kind, body), "", scope));
                         }

-                        &ControlFlow::Return { pos, result } => {
+                        &ControlFlow::Return { pos: _pos, result } => {
                             writeln!(
                                 ctx.out,
-                                "{}// Rule at {}.",
+                                "{}",
                                 &ctx.indent,
-                                pos.pretty_print_line(&self.files)
                             )?;
                             write!(ctx.out, "{}", &ctx.indent)?;
                             match ret_kind {

2. The behavior could be enabled/disabled based on a crate feature or environment variable.
3. The path could be made relative to CARGO_MANIFEST_DIR; alternatively only the basename of the source file could be retained and written to the generated files.

Not very familiar with the crate so can't make an educated decision! But this would be a nice improvement in the way of system-independent reproducible builds.

Wasmtime GitHub notifications bot (Nov 05 2024 at 08:44):

nmattia edited issue #9553:

Feature

It would be great to avoid printing full paths of source files in the generated code.

Benefit

This would allow for better or more correct caching in third party build systems like Bazel or Nix.

Implementation

1. The naive solution is to remove references to the source files:

diff --git a/cranelift/codegen/meta/src/srcgen.rs b/cranelift/codegen/meta/src/srcgen.rs
index d3c321e5b..5b94ddd19 100644
--- a/cranelift/codegen/meta/src/srcgen.rs
+++ b/cranelift/codegen/meta/src/srcgen.rs
@@ -94,7 +94,6 @@ impl Formatter {
         directory: &std::path::Path,
     ) -> Result<(), error::Error> {
         let path = directory.join(&filename);
-        eprintln!("Writing generated file: {}", path.display());
         let mut f = fs::File::create(path)?;

         for l in self.lines.iter().map(|l| l.as_bytes()) {
diff --git a/cranelift/isle/isle/src/codegen.rs b/cranelift/isle/isle/src/codegen.rs
index 158285832..d292e43c0 100644
--- a/cranelift/isle/isle/src/codegen.rs
+++ b/cranelift/isle/isle/src/codegen.rs
@@ -127,9 +127,6 @@ impl<'a> Codegen<'a> {
             "// Generated automatically from the instruction-selection DSL code in:",
         )
         .unwrap();
-        for file in &self.files.file_names {
-            writeln!(code, "// - {file}").unwrap();
-        }

         if !options.exclude_global_allow_pragmas {
             writeln!(
@@ -641,12 +638,11 @@ impl<L: Length, C> Length for ContextIterWrapper<L, C> {{
                             stack.push((Self::validate_block(ret_kind, body), "", scope));
                         }

-                        &ControlFlow::Return { pos, result } => {
+                        &ControlFlow::Return { pos: _pos, result } => {
                             writeln!(
                                 ctx.out,
-                                "{}// Rule at {}.",
+                                "{}",
                                 &ctx.indent,
-                                pos.pretty_print_line(&self.files)
                             )?;
                             write!(ctx.out, "{}", &ctx.indent)?;
                             match ret_kind {

2. The behavior could be enabled/disabled based on a crate feature or environment variable.
3. The path could be made relative to CARGO_MANIFEST_DIR; alternatively only the basename of the source file could be retained and written to the generated files.

Not very familiar with the crate so can't make an educated decision! But this would be a nice improvement in the way of system-independent reproducible builds.

Wasmtime GitHub notifications bot (Nov 05 2024 at 18:41):

cfallin commented on issue #9553:

Hi @nmattia -- thanks for filing this issue!

I don't think we will remove the paths altogether -- let me explain a bit more why. We emit these paths in the generated Rust code to help us debug and to see where rule-matching cases are defined in the original source. It was an explicit design goal for the output of the ISLE metacompiler to be relatively comprehensible to humans. The ability to refer back to source locations is useful for this.

That said, I'd be happy to review a PR that makes the paths relative to the source root (your option 3).

I'm also a bit curious about your use-case. You say "more correct caching" -- do you mean, higher cache hit-rate for a cache shared across many builds at different paths? We have not made it an explicit design goal to avoid, e.g., absolute paths in comments, though we do take care to ensure the final built code is semantically deterministic (i.e. our only variances should be in comments). I think it'd be reasonable to aim for this tighter intermediate-textual-form definition of determinism, just want to clarify the exact requirement.

Wasmtime GitHub notifications bot (Nov 06 2024 at 19:07):

fitzgen commented on issue #9553:

FWIW, I think we do (or did at one point do) this kind of prefix removal in cranelift's build script or the meta crate.

Wasmtime GitHub notifications bot (Nov 07 2024 at 09:25):

nmattia commented on issue #9553:

We emit these paths in the generated Rust code to help us debug

That makes complete sense!

I'd be happy to review a PR that makes the paths relative to the source root

I'll prepare something and send it your way then :)

higher cache hit-rate for a cache shared across many builds at different paths?

Yes, that's right! Some build systems read and hash inputs to see if any of the outputs that depend on said inputs need to be rebuilt -- unlike Make for instance, which uses mtime. So when some of those inputs (generated code with changing comments) change, the previously built outputs are discarded and need to be rebuilt, even though the changed comments should not have an impact on the actual built code.

Last updated: Nov 22 2024 at 16:03 UTC