Closed Bug 1455799 Opened Last year Closed Last year

Errors during webidl code generation don't cause the build to fail

Categories

(Firefox Build System :: General, defect, major)

3 Branch
defect
Not set
major

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: mshal)

References

Details

Attachments

(1 file)

This is clearly a regression but I don't know how old...

STEPS TO REPRODUCE:

1)  Build a clean tree.
2)  Edit dom/webidl/CSS.webidl and add some stray characters at
    the end of the file.  Basically, type "abcdef" after the last ';'.
3)  mach build

EXPECTED RESULTS: Build fails.

ACTUAL RESULTS: A bunch of error messages are shown, but the build does not stop at them and keeps going.  At the end it says "Your build was successful!".  The  return status is success (so for example "mach build && echo OK" echoes "OK").
It looks like this is because of the "-include codegen.pp" line in dom/bindings/Makefile.in, which was changed from a regular "include" in bug 1378965 to avoid the error message about not finding a codegen.pp file on the first pass of parsing the Makefile. (The order of operations here is that make parses the objdir/dom/bindings/Makefile and tries to include codegen.pp, which doesn't exist yet, so the rule to create codegen.pp executes, and then make reloads the Makefile). Unfortunately it seems that -include not only suppresses the error message, but also ignores any errors in remaking the codegen.pp file. From [1]:

  -include filenames…

   This acts like include in every way except that there is no error (not even a warning) if any of the filenames (or any prerequisites of any of the filenames) do not exist or cannot be remade.

So I think we either have to back out bug 1378965 or change the rule structure so that codegen.pp is generated as a side-effect of a normal rule, more similar to how g++ generates .pp files as a side-effect of the rule to create .o files.

[1] https://www.gnu.org/software/make/manual/html_node/Include.html
Hmm.  Is codegen.pp right now the "target that determines whether bindings have been generated"?

It seems like we could certainly introduce a dummy target for that...
Blocks: 1378965
OK, this is causing me daily pain, because I will do a build and it will claim to be done but then when I try to run it it either doesn't run or doesn't include my changes...  Nathan, Mike, do either of you plan to work on this soon?  If not, I'll probably just try to fix it...
Flags: needinfo?(mshal)
Yep, I'm actually working on it now - sorry I didn't update the bug. I should have a fix for review today.
Assignee: nobody → mshal
Flags: needinfo?(mshal)
Attachment #8972097 - Flags: feedback?(bzbarsky)
Comment on attachment 8972097 [details]
Bug 1455799 - Make an explicit webidl export target;

https://reviewboard.mozilla.org/r/240822/#review246548

Looks like this works in my local testing.  Thank you!

::: commit-message-63519:13
(Diff revision 1)
> +non-existent file, but also any errors in regenerating the target from
> +the webidl py_action.
> +
> +Instead we can make a separate PHONY target for webidl generation, and
> +include the codegen.pp that's generated as a side-effect of the
> +py_actio. This way make will fail properly if the webidl generation

py_action
Attachment #8972097 - Flags: review+
Attachment #8972097 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Comment on attachment 8972097 [details]
Bug 1455799 - Make an explicit webidl export target;

https://reviewboard.mozilla.org/r/240822/#review246570

::: dom/bindings/Makefile.in:36
(Diff revision 2)
> +export:: webidl
> +
> +# codegen.pp is created as a side-effect of the webidl action
>  -include codegen.pp
>  
> -codegen.pp: $(codegen_dependencies)
> +.PHONY: webidl

The .PHONY target here is making no-op builds a lot slower in export... can't we change this rule be a dummy tracking file that export depends on?

::: dom/bindings/Makefile.in:39
(Diff revision 2)
>  -include codegen.pp
>  
> -codegen.pp: $(codegen_dependencies)
> +.PHONY: webidl
> +webidl: $(codegen_dependencies)
>  	$(call py_action,webidl,$(srcdir))
>  	@$(TOUCH) $@

If we do end up needing a phony rule for this there's no sense in touching the output here.
Attachment #8972097 - Flags: review?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #8)
> Comment on attachment 8972097 [details]
> Bug 1455799 - Make an explicit webidl export target;
> 
> https://reviewboard.mozilla.org/r/240822/#review246570
> 
> ::: dom/bindings/Makefile.in:36
> (Diff revision 2)
> > +export:: webidl
> > +
> > +# codegen.pp is created as a side-effect of the webidl action
> >  -include codegen.pp
> >  
> > -codegen.pp: $(codegen_dependencies)
> > +.PHONY: webidl
> 
> The .PHONY target here is making no-op builds a lot slower in export...
> can't we change this rule be a dummy tracking file that export depends on?

Gah, good catch. I've made a dummy webidl.stub file instead of a PHONY target. I should've also updated the codegen.pp file so that the dependencies it generates are for the new target.
Comment on attachment 8972097 [details]
Bug 1455799 - Make an explicit webidl export target;

https://reviewboard.mozilla.org/r/240822/#review246602
Attachment #8972097 - Flags: review?(cmanchester) → review+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/359cecde7c3e
Make an explicit webidl export target; r=bz,chmanchester
https://hg.mozilla.org/mozilla-central/rev/359cecde7c3e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.