935305 - Preprocessor doesn't track dependencies

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

We have many preprocessed files containing #include statements, and we don't handle these dependencies. In some places, those dependencies are handled manually, and that's error prone.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Move preprocessor to mozbuild.action

I skipped improving makefiles further than changing > to -o and consequently removing the corresponding mkdirs.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Move preprocessor to mozbuild.action

With typo fix.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Track preprocessor output dependencies

This adds the option to preprocessor.py and uses it for PP_TARGETS, with some safeguards.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Move preprocessor to mozbuild.action

With one less mkdir-like command, and rebased on top of 935387.

Comment 6

[Axel Hecht [:Pike]]

I'd like to be able to continue to use Preprocessor.py as main py file, I at least have external tools that do so.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 827851 [details] [diff] [review]
Move preprocessor to mozbuild.action

Review of attachment 827851 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

What do you think about rolling expression.py into preprocessor.py? Do we use expression.py anywhere else?

Will likely grant r+ once I have answers to questions.

::: config/Makefile.in
@@ +73,5 @@
>    $(call mkdir_deps,system_wrappers) \
>    $(NULL)
>  
>  export:: $(export-preqs)
> +	$(PYTHON) -m mozbuild.action.preprocessor $(DEFINES) $(ACDEFINES) \

Why no py_action?

::: config/makefiles/functions.mk
@@ +29,5 @@
>  #       $(call py_action,purge_manifests,_build_manifests/purge/foo.manifest)
>  ifdef .PYMAKE
>  py_action = %mozbuild.action.$(1) main $(2)
>  else
> +py_action = PYTHONDONTWRITEBYTECODE=1 $(PYTHON) -m mozbuild.action.$(1) $(2)

Why did you add PYTHONDONTWRITEBYTECODE? The .pyc files are annoying, sure, but we're already dealing with them elsewhere in the tree. Which reminds me, I /really/ want to fix that...

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 827851 [details] [diff] [review]
> Move preprocessor to mozbuild.action
> 
> Review of attachment 827851 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> What do you think about rolling expression.py into preprocessor.py? Do we
> use expression.py anywhere else?

No preference, and considering the patch removes Expression.py and nothing complains...

> Will likely grant r+ once I have answers to questions.
> 
> ::: config/Makefile.in
> @@ +73,5 @@
> >    $(call mkdir_deps,system_wrappers) \
> >    $(NULL)
> >  
> >  export:: $(export-preqs)
> > +	$(PYTHON) -m mozbuild.action.preprocessor $(DEFINES) $(ACDEFINES) \
> 
> Why no py_action?

| foo
pymake is not going to like that.

> ::: config/makefiles/functions.mk
> @@ +29,5 @@
> >  #       $(call py_action,purge_manifests,_build_manifests/purge/foo.manifest)
> >  ifdef .PYMAKE
> >  py_action = %mozbuild.action.$(1) main $(2)
> >  else
> > +py_action = PYTHONDONTWRITEBYTECODE=1 $(PYTHON) -m mozbuild.action.$(1) $(2)
> 
> Why did you add PYTHONDONTWRITEBYTECODE? The .pyc files are annoying, sure,
> but we're already dealing with them elsewhere in the tree. Which reminds me,
> I /really/ want to fix that...

I moved it here because it was in one of the invokations i replaced and i thought "meh, what the hell".

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 827844 [details] [diff] [review]
Track preprocessor output dependencies

Review of attachment 827844 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/preprocessor.py
@@ +80,5 @@
>      self.setMarker('#')
>      self.LE = '\n'
>      self.varsubst = re.compile('@(?P<VAR>\w+)@', re.U)
> +    self.depfile = None
> +    self.includes = []

Use a set instead?

@@ +167,5 @@
>              raise
> +      return open(path, 'wb')
> +
> +    p = self.getCommandLineParser()
> +    (options, args) = p.parse_args(args=args)

Nit: remove the extra ()

@@ +190,1 @@
>      pass

Kill this pass while you are here.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 827851 [details] [diff] [review]
Move preprocessor to mozbuild.action

Review of attachment 827851 [details] [diff] [review]:
-----------------------------------------------------------------

r+ conditional on removing PYTHONDONTWRITEBYTECODE.

I don't like littering .pyc files. But, Python normally does the right thing and we've been living with .pyc files without any major issue, so I don't see a need to stop getting the slight perf boost.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Additional patch to fold in the preprocessor move

This folds expression in preprocessor and makes python/mozbuild/mozbuild/preprocessor.py independently callable, to please people who would want to use it without the virtualenv.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Track preprocessor output dependencies

Addresses review comments, rebased on part 1 + new additional patch, and kept preprocessor.py working independently even when copied out of python/mozbuild/mozbuild.

Comment 13

[Mike Hommey [:glandium]]

Attachment: One more additional patch to fold to not fail in subtle ways when used as pymake native command.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Track preprocessor output dependencies

Refreshed and added a close for the depfile.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 829203 [details] [diff] [review]
One more additional patch to fold to not fail in subtle ways when used as pymake native command.

Review of attachment 829203 [details] [diff] [review]:
-----------------------------------------------------------------

Boo for not cleaning up after oneself! This almost certainly points to a test gap. Glad to see it fixed.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 829204 [details] [diff] [review]
Track preprocessor output dependencies

Review of attachment 829204 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +1625,5 @@
> +    $(eval $(file): FORCE) \
> +  ) \
> +)
> +
> +MDDEPEND_FILES := $(strip $(wildcard $(addprefix $(MDDEPDIR)/,$(addsuffix .pp,$(notdir $(PP_TARGETS_ALL_RESULTS))))))

:= instead of += seems fragile.

::: python/mozbuild/mozbuild/preprocessor.py
@@ +416,5 @@
> +      try:
> +        from makeutil import Makefile
> +      except:
> +        raise Preprocessor.Error(self, "--depend requires the "
> +                                       "mozbuild.makeutil module", None)

Were we really hitting this? Boo.

@@ +425,5 @@
>          self.do_include(f, False)
>        self.warnUnused(f)
> +      if self.depfile:
> +        mk = Makefile()
> +        mk.create_rule([options.output]).add_dependencies(self.includes)

sorted(self.includes)

Comment 17

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #16)
> > +MDDEPEND_FILES := $(strip $(wildcard $(addprefix $(MDDEPDIR)/,$(addsuffix .pp,$(notdir $(PP_TARGETS_ALL_RESULTS))))))
> 
> := instead of += seems fragile.

See the other things involving MDDEPEND_FILES in rules.mk, we're treating each assignment as a separate variable.

> ::: python/mozbuild/mozbuild/preprocessor.py
> @@ +416,5 @@
> > +      try:
> > +        from makeutil import Makefile
> > +      except:
> > +        raise Preprocessor.Error(self, "--depend requires the "
> > +                                       "mozbuild.makeutil module", None)
> 
> Were we really hitting this? Boo.

We're not. This allows preprocessor.py to be copied and used elsewhere without requiring that module. You just don't get the --depend feature if you do.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #17)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > > +MDDEPEND_FILES := $(strip $(wildcard $(addprefix $(MDDEPDIR)/,$(addsuffix .pp,$(notdir $(PP_TARGETS_ALL_RESULTS))))))
> > 
> > := instead of += seems fragile.
> 
> See the other things involving MDDEPEND_FILES in rules.mk, we're treating
> each assignment as a separate variable.

(although, the other two are mutually exclusive)

Comment 19

[Gregory Szorc [:gps]]

Fair enough. Land away.

Comment 20

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/484945495f95
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ffa28bd697

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/484945495f95
https://hg.mozilla.org/mozilla-central/rev/26ffa28bd697

Comment 22

[Axel Hecht [:Pike]]

Filed https://github.com/Pike/font-tool/issues/4 for my external use-case.