Closed Bug 935305 Opened 6 years ago Closed 6 years ago

Preprocessor doesn't track dependencies

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files, 4 obsolete files)

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.
I skipped improving makefiles further than changing > to -o and consequently removing the corresponding mkdirs.
Attachment #827738 - Flags: review?(gps)
With typo fix.
Attachment #827761 - Flags: review?(gps)
Attachment #827738 - Attachment is obsolete: true
Attachment #827738 - Flags: review?(gps)
This adds the option to preprocessor.py and uses it for PP_TARGETS, with some safeguards.
Attachment #827844 - Flags: review?(gps)
Depends on: 935387
With one less mkdir-like command, and rebased on top of 935387.
Attachment #827851 - Flags: review?(gps)
Attachment #827761 - Attachment is obsolete: true
Attachment #827761 - Flags: review?(gps)
Duplicate of this bug: 924343
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 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...
Attachment #827851 - Flags: review?(gps) → feedback+
(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 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.
Attachment #827844 - Flags: review?(gps) → review+
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.
Attachment #827851 - Flags: review+
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.
Attachment #828953 - Flags: review?(gps)
Depends on: 934864
Addresses review comments, rebased on part 1 + new additional patch, and kept preprocessor.py working independently even when copied out of python/mozbuild/mozbuild.
Attachment #828957 - Flags: review?(gps)
Attachment #827844 - Attachment is obsolete: true
Refreshed and added a close for the depfile.
Attachment #829204 - Flags: review?(gps)
Attachment #828957 - Attachment is obsolete: true
Attachment #828957 - Flags: review?(gps)
Attachment #828953 - Flags: review?(gps) → review+
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.
Attachment #829203 - Flags: review?(gps) → review+
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)
Attachment #829204 - Flags: review?(gps) → review+
(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.
(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)
Fair enough. Land away.
https://hg.mozilla.org/mozilla-central/rev/484945495f95
https://hg.mozilla.org/mozilla-central/rev/26ffa28bd697
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 937024
Blocks: 937212
Filed https://github.com/Pike/font-tool/issues/4 for my external use-case.
Duplicate of this bug: 837792
Whiteboard: [qa-]
Blocks: 1058036
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.