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

Turn duplicate rules for the same target into an error #931

Closed
5 tasks done
nico opened this issue Mar 14, 2015 · 2 comments · Fixed by #2356
Closed
5 tasks done

Turn duplicate rules for the same target into an error #931

nico opened this issue Mar 14, 2015 · 2 comments · Fixed by #2356
Milestone
1.12.0

Comments

@nico
Copy link
Collaborator

nico commented Mar 14, 2015 •
edited by jhasse

If multiple build edges generate the same output, ninja currently warns with

ninja: warning: multiple rules generate $name. build will not be correct; continuing anyway

Internally, ninja just ignores all but the last rule. In practice, this usually happens if people accidentally repeat a rule in their meta-buildsystem. But for rules with multiple targets, ninja's current handling of this can lead to inconsistent build graphs that can lead to crashes (see e.g. #867). Also, some people mistake this diagnostic for a bug in ninja instead of a bug in their build files (e.g. #543 last comment). Arguably, ninja could try to check if the two rules are exactly the same and not complain in that case, but

  1. That's something the generator could do
  2. It's not always obvious – the commands could be cd ../.. && foo/bar.sh and cd ../../.. && baz/foo/bar.sh, or they could set slightly different env vars (which happen to not make a difference in practice, but ninja can't know that)

So this should be an error, not a warning. However, this is something that happens in practice every now and then (examples: http://reviews.llvm.org/D6166#109619 http://crbug.com/351052), so the rollout for this should look something like this:

  1. Add a flag to turn this warning into an error, but don't set it by default (--dupeedge=[warn|error]?), make a release with this. (Mention the flag name in the warning text.)
  2. File bugs with all known generators to reject duplicate rules for the same target (up to generators to allow this if the two rules effectively are the same – generators are in a better position to make that decision. If they opt to do that, they still should only write one of these rules). Wait for this to be implemented everywhere, and to make it into releases.
  3. Flip the flag default to error, but keep the flag around to switch back to warning, keep mentioning the flag in the error message, and in --help output. Make another release.
  4. Remove flag from error message and help output, but keep honoring it. Make another release.
  5. Eventually, remove the flag.

Not sure if we ever actually want to do 5, but the rest all seem like Obviously Good steps.
@nico
Copy link
Collaborator Author

nico commented Mar 19, 2015

#940 was another bug in this area.

nico added a commit to nico/ninja that referenced this issue Mar 24, 2015
@nico
Add an opt-in flag to make duplicate edges an error (-w dupeedge=err).
ef12be0 
This is step 1 on ninja-build#931.  Duplicated edges will become an error by default in
the future.
@nico nico mentioned this issue Mar 24, 2015
Merged
nico added a commit to nico/ninja that referenced this issue Mar 24, 2015
@nico
Add an opt-in flag to make duplicate edges an error (-w dupeedge=err).
ab28060 
This is step 1 on ninja-build#931.  Duplicated edges will become an error by default in
the future.
nico added a commit to nico/ninja that referenced this issue Mar 24, 2015
@nico
Add an opt-in flag to make duplicate edges an error (-w dupbuild=err).
3b3876c 
This is step 1 on ninja-build#931.  Duplicated edges will become an error by default in
the future.
nico added a commit to nico/ninja that referenced this issue Mar 24, 2015
@nico
Add an opt-in flag to make duplicate edges an error (-w dupbuild=err).
92b74c3 
This is step 1 on ninja-build#931.  Duplicated edges will become an error by default in
the future.
@nico nico mentioned this issue Sep 7, 2017
Closed
@nico
Copy link
Collaborator Author

nico commented Apr 5, 2018

RfC to make this err by default from a while ago (no concerns): https://groups.google.com/forum/#!topic/ninja-build/ob5vhaGvvTc

nico added a commit to nico/ninja that referenced this issue Apr 5, 2018
@nico
make -w dupbuild default to err
42eefc8 
You can still opt out of this by passing `-w dupbuild=warn`.
But if you're getting this diagnostic, your build files are incorrect
and you should ideally just fix them.

This is step 3 on ninja-build#931
I sent an RfC to ninja-build a few months ago; nobody objected.
@nico nico mentioned this issue Apr 5, 2018
Merged
projectgus pushed a commit to projectgus/ninja that referenced this issue Apr 26, 2018
@nico @projectgus
make -w dupbuild default to err
3673ed9 
You can still opt out of this by passing `-w dupbuild=warn`.
But if you're getting this diagnostic, your build files are incorrect
and you should ideally just fix them.

This is step 3 on ninja-build#931
I sent an RfC to ninja-build a few months ago; nobody objected.
@jhasse jhasse mentioned this issue Nov 12, 2018
Closed
kdesysadmin pushed a commit to KDE/krita that referenced this issue Mar 9, 2019
@hallarempt
Rename this test
e9b99ee 
Because the Ninja people, in their infinite meddlesomeness have
decided that you shouldn't have two unittests with the same name
in a body of code (ninja-build/ninja#931)
we need to rename one of these tests to something else. Whatever.
@eszlari eszlari mentioned this issue Apr 7, 2020
Closed
@jhasse jhasse added this to the 1.11.0 milestone May 18, 2020
jhasse added a commit that referenced this issue Mar 20, 2021
@jhasse
Remove -w dupbuild from error message and help output
db6c4ac 
This is step 4 on #931.
@jhasse jhasse modified the milestones: 1.11.0, 1.12.0 Mar 20, 2021
kasbah pushed a commit to kasbah/ninja that referenced this issue Mar 23, 2021
@jhasse @kasbah
Remove -w dupbuild from error message and help output
66203d3 
This is step 4 on ninja-build#931.
rascani pushed a commit to rascani/ninja that referenced this issue Apr 29, 2021
@jhasse @rascani
Remove -w dupbuild from error message and help output
1db5109 
This is step 4 on ninja-build#931.
@ksherlock ksherlock mentioned this issue Jun 14, 2021
Open
@jhasse jhasse mentioned this issue Nov 29, 2023
Merged
jhasse added a commit to jhasse/ninja that referenced this issue Nov 29, 2023
@jhasse
Remove -w dupbuild completely, always error on duplicate edges
8f47d5a 
Step 5, fixes ninja-build#931.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.12.0
Development

Successfully merging a pull request may close this issue.

Remove -w dupbuild completely, always error on duplicate edges jhasse/ninja
2 participants
@nico @jhasse