Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Path expansion in builds doesn't take take build variables in context #39

Open
evmar opened this issue Apr 5, 2022 · 9 comments
Open

Path expansion in builds doesn't take take build variables in context #39

evmar opened this issue Apr 5, 2022 · 9 comments
Labels
ninja incompatibility Behaviors that are incompatible with Ninja

Comments

@evmar
Copy link
Owner

evmar commented Apr 5, 2022 •
edited
Loading

In Ninja, the below refers to the file foo.3.

build foo.$bar: baz ...
  bar = 3

In n2 this doesn't. n2 aggressively expands variables while parsing, which is to say that in the above it never even generates intermediate data like the parse of foo.$bar, but rather expands $bar right as it sees it and then interns the path, and even reuses that buffer to parse/canonicalize the next path.

This is fixable but I'm kinda hesitant. To implement the Ninja behavior we'd instead need to build up an array of all the paths and then expand them once we've parsed through the variables. (Edit: another possibly costlier idea, we could skip forward to the variables, parse them, then go back and parse the paths again.)

The fact that builds seem to currently work suggests that maybe existing projects don't depend on this?
@evmar evmar added the ninja incompatibility Behaviors that are incompatible with Ninja label Apr 5, 2022
@dae
Copy link
Contributor

dae commented Jun 15, 2023

For what it's worth, this was one of the two issues I faced in our build when trying to switch over to n2 from ninja. Our build generator would define things like $builddir at the top of build.ninja, and then in individual build steps, it would use variables that made use of them, eg out_dir = $builddir/some/path.

@dae dae mentioned this issue Jun 15, 2023
Closed
@evmar
Copy link
Owner Author

evmar commented Jun 15, 2023

Dang, that's a pretty strong argument that we ought to fix this. Unfortunately it will likely end up fairly costly. :( n2 is very aggressive about trying to do work incrementally to avoid needing to buffer stuff while parsing...

@dae
Copy link
Contributor

dae commented Jun 15, 2023

Might be worth waiting around to see if anyone else hits this - it was fairly easy to work around in our case, and we may be an outlier. Doing a couple of string replacements in the build generator does seem to make more sense that pushing the work to build time.

@evmar
Copy link
Owner Author

evmar commented Aug 29, 2023

For what it's worth, this was one of the two issues I faced in our build when trying to switch over to n2 from ninja. Our build generator would define things like $builddir at the top of build.ninja, and then in individual build steps, it would use variables that made use of them, eg out_dir = $builddir/some/path.

Rereading this, I think this ought to be something that currently does work with n2. The specific issue here is instead about how in some (rare) cases Ninja variable expansion refers to a variable that it defined textually below where it's referenced, while n2 expands things roughly top-down. So I think that doesn't describe what you encountered.

@dae
Copy link
Contributor

dae commented Aug 30, 2023

I might be missing something. Here's an example:

builddir = /home/dae/Local/build/anki
runner = $builddir/rust/release/runner

rule CargoBuild
  command = $runner run cargo build $release_arg $target_arg --locked $extra_args
  restat = 1
  hide_success = 1
  hide_last_line = 0

build  | $builddir/rust/debug/configure: CargoBuild  | .cargo/config.toml Cargo.lock build/configure/Cargo.toml build/configure/src/aqt.rs build/configure/src/bundle.rs build/configure/src/main.rs build/configure/src/platform.rs build/configure/src/pylib.rs build/configure/src/python.rs build/configure/src/rust.rs build/configure/src/web.rs build/ninja_gen/Cargo.toml build/ninja_gen/src/action.rs build/ninja_gen/src/archives.rs build/ninja_gen/src/build.rs build/ninja_gen/src/cargo.rs build/ninja_gen/src/command.rs build/ninja_gen/src/configure.rs build/ninja_gen/src/copy.rs build/ninja_gen/src/git.rs build/ninja_gen/src/hash.rs build/ninja_gen/src/input.rs build/ninja_gen/src/lib.rs build/ninja_gen/src/node.rs build/ninja_gen/src/protobuf.rs build/ninja_gen/src/python.rs build/ninja_gen/src/render.rs build/ninja_gen/src/rsync.rs build/ninja_gen/src/sass.rs build/runner/Cargo.toml build/runner/build.rs build/runner/src/archive.rs build/runner/src/build.rs build/runner/src/bundle/artifacts.rs build/runner/src/bundle/binary.rs build/runner/src/bundle/folder.rs build/runner/src/bundle/mod.rs build/runner/src/main.rs build/runner/src/paths.rs build/runner/src/pyenv.rs build/runner/src/rsync.rs build/runner/src/run.rs build/runner/src/yarn.rs rust-toolchain.toml
  configure = $builddir/rust/debug/configure
  description = build:configure_bin
  extra_args = -p configure
  release_arg = 
  target_arg = 

rule ConfigureBuild
  command = $runner run $cmd
  restat = 1
  generator = 1
  hide_success = 1
  hide_last_line = 0

build  | $builddir/build.ninja: ConfigureBuild  | $builddir/env $builddir/rust/debug/configure .git .version
  cmd = $builddir/rust/debug/configure
  description = build:configure

This fails with:

failed: build:configure
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DidNotExecute { cmdline: "/rust/debug/configure", source: Os { code: 2, kind: NotFound, message: "No such file or directory" } }', build/runner/src/run.rs:86:30

@evmar evmar mentioned this issue Aug 30, 2023
Closed
@evmar
Copy link
Owner Author

evmar commented Aug 30, 2023

Thanks, opened #83 about this one.

@evmar
Copy link
Owner Author

evmar commented Aug 30, 2023

(And now fixed, maybe.)

@dae
Copy link
Contributor

dae commented Aug 30, 2023

Can confirm that's fixed it - thank you!

@evmar evmar mentioned this issue Dec 26, 2023
Closed
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
5a95522 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91, and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
eec13b0 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
977324d 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
7b64b7d 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
3f797e6 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
de42648 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
@Colecf Colecf mentioned this issue Dec 29, 2023
Merged
Colecf added a commit to Colecf/n2 that referenced this issue Dec 31, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
9dffdcb 
This fixes an incompatibility with ninja.

I've also refactored Env to make it clearer and remove the TODO(evmar#83),
but I actually think we could do signifigant further cleanup. Only
rules should require EvalStrings, global variables and build bindings
can be evaluated as soon as they're read, although maybe that would
change with subninjas. Also, rules currently parse their variables as
EvalString<String>, but I think that could be changed to
EvalString<&'text str> if we hold onto the byte buffers of all the
included files until the parsing is done.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Dec 31, 2023
@Colecf
Evaluate build paths in the context of the build's bindings
c13f859 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Jan 2, 2024
@Colecf
Evaluate build paths in the context of the build's bindings
8e61026 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Jan 3, 2024
@Colecf
Evaluate build paths in the context of the build's bindings
fd10e19 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Jan 4, 2024
@Colecf
Evaluate build paths in the context of the build's bindings
9831750 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes evmar#91 and evmar#39.
Colecf added a commit to Colecf/n2 that referenced this issue Jan 5, 2024
@Colecf
Evaluate build paths in the context of the build's bindings
d510d1d 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes evmar#91 and evmar#39.
evmar pushed a commit that referenced this issue Jan 18, 2024
@Colecf @evmar
Evaluate build paths in the context of the build's bindings
909ac60 
This fixes an incompatibility with ninja.

I've also moved a bunch of variable evaluations out of the
parser and into the loader, in preparation for parsing the
build file in multiple threads, and then only doing the
evaluations after all the chunks of the file have been
parsed.

Fixes #91 and #39.
@Colecf
Copy link
Contributor

Colecf commented Jan 19, 2024

This should be completed as of #94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ninja incompatibility Behaviors that are incompatible with Ninja
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@evmar @dae @Colecf