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.

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

Variable references in build inputs don't take into account the build's bindings #91

Closed
Colecf opened this issue Dec 25, 2023 · 5 comments · Fixed by #94
Closed

Variable references in build inputs don't take into account the build's bindings #91

Colecf opened this issue Dec 25, 2023 · 5 comments · Fixed by #94

Comments

@Colecf
Copy link
Contributor

Colecf commented Dec 25, 2023

rule touch
    command = touch $out

rule copy
    command = cp $in $out

build out/a: copy ${my_dep}
    my_dep = out/b

build out/b: touch

default out/a

This build file works in ninja, but not n2. Android has this case. I've only found one occurance that was trivial to remove so far, but I haven't gotten a full build working yet. If possible, maybe it's better not to fix it if it allows faster parsing.

@evmar
Copy link
Owner

evmar commented Dec 26, 2023

This is #39 and it is dumb that I didn't make it work, but I was greedy.

The reason for my greed is that n2 currently expands everything right while it parses, so when it sees ${my_dep} it wants to expand it to the final value without waiting to parse down through the definition of my_dep = . This means it never needs to buffer any of these intermediate strings. But it also means it's just plain incorrect relative to Ninja.

Now that I look at your example, I wonder if I could do some trick around assuming things will go the fast way and then if I see a variable reference later going back and re-expanding. Or if I should just buffer everything anyway. It would probably help for me to actually profile before I micro-optimize in this manner.

@evmar
Copy link
Owner

evmar commented Dec 26, 2023

Relevant expansion inline code, I think maybe that ought to consult more scopes than it currently does too...

@Colecf
Copy link
Contributor Author

Colecf commented Dec 26, 2023

Oh, feel free to close as a duplicate then. Interestingly, this issue also causes parse errors:

rule touch
    command = touch $out

rule copy
    command = cp $in $out

my_var = out/b

build out/a: copy out/b | ${my_dep} out/d
    my_dep = out/c

build out/b: touch
build out/c: touch
build out/d: touch

default out/a


Produces:

n2: error: parse error: expected '\n', got ' '
build.ninja:9: build out/a: copy out/b | ${my_dep} out/...
                                                  ^

I actually think this will need to be properly fixed for the android build. Not all of these search results, but many of them, suffer from this issue.

Colecf added a commit to Colecf/n2 that referenced this issue Dec 29, 2023
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
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
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
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
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
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
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
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
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
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
Copy link
Owner

evmar commented Jan 4, 2024

(Sorry for not responding yet -- your other patches were obvious merges, but this one is more subtle so I want to give it proper attention and I have some life stuff going on the next week or so.)

Colecf added a commit to Colecf/n2 that referenced this issue Jan 4, 2024
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
Copy link
Contributor Author

Colecf commented Jan 4, 2024

Thanks for the update, I'll hold off on new PRs for now then.

Colecf added a commit to Colecf/n2 that referenced this issue Jan 5, 2024
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 evmar closed this as completed in #94 Jan 18, 2024
evmar pushed a commit that referenced this issue Jan 18, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants