Do not regenerate buildid.h for every tup build

RESOLVED FIXED in Firefox 61

Status

enhancement
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks 1 bug)

3 Branch
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

We need to bring the fix for bug 1298328 to tup.
Assignee: nobody → cmanchester
Comment hidden (mozreview-request)

Comment 2

Last year
mozreview-review
Comment on attachment 8961121 [details]
Bug 1445398 - Do not re-generate buildid.h for every Tup build.

https://reviewboard.mozilla.org/r/229922/#review235854

::: python/mozbuild/mozbuild/backend/tup.py:191
(Diff revision 1)
>  
>          # These are 'group' dependencies - All rules that list these as an output
>          # will be built before any rules that list this as an input.
>          self._installed_idls = '$(MOZ_OBJ_ROOT)/<installed-idls>'
>          self._installed_files = '$(MOZ_OBJ_ROOT)/<installed-files>'
> +        # The preprocessor can hide dependencies from Tup in some cases (when

The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group.

::: python/mozbuild/mozbuild/backend/tup.py:518
(Diff revision 1)
>          # '%%' in the Tupfile.
>          marker = '%%' if target.endswith('.css') else '#'
>  
>          cmd = self._py_action('preprocessor')
>          cmd.extend([shell_quote(d) for d in backend_file.defines])
> -        cmd.extend(['$(ACDEFINES)', '%f', '-o', '%o', '--marker=%s' % marker])
> +        cmd.extend(['$(ACDEFINES)', input_file, '-o', '%o', '--marker=%s' % marker])

You could keep the '%f' here and instead have self._early_generated_files in extra_inputs. The extra_inputs do not show up in %f while inputs do. (Similarly for extra_outputs and outputs in %o).
Attachment #8961121 - Flags: review?(mshal)
Assignee

Comment 3

Last year
mozreview-review-reply
Comment on attachment 8961121 [details]
Bug 1445398 - Do not re-generate buildid.h for every Tup build.

https://reviewboard.mozilla.org/r/229922/#review235854

> The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group.

It's an issue for application.ini as well as some other preprocessed files that include these files. Unfortunately adding these headers as inputs doesn't quite work because it also feeds them to the preprocessor (and we don't have a facility to do something like this for regular preprocessed files).

> You could keep the '%f' here and instead have self._early_generated_files in extra_inputs. The extra_inputs do not show up in %f while inputs do. (Similarly for extra_outputs and outputs in %o).

Thanks for the tip, will do!
Comment hidden (mozreview-request)
(In reply to Chris Manchester (:chmanchester) from comment #3)
> Comment on attachment 8961121 [details]
> Bug 1445398 - Do not re-generate buildid.h for every Tup build.
> 
> https://reviewboard.mozilla.org/r/229922/#review235854
> 
> > The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group.
> 
> It's an issue for application.ini as well as some other preprocessed files
> that include these files. Unfortunately adding these headers as inputs
> doesn't quite work because it also feeds them to the preprocessor (and we
> don't have a facility to do something like this for regular preprocessed
> files).

Oh, looks like platform.ini is the other one? I guess we could call preprocessor.py for that as a GENERATED_FILES so we can specify buildid.h / source-repo.h as inputs similar to application.ini instead of having it as a FINAL_TARGET_PP_FILES. We can always revisit that later though - no need to block on this.

Comment 6

Last year
mozreview-review
Comment on attachment 8961121 [details]
Bug 1445398 - Do not re-generate buildid.h for every Tup build.

https://reviewboard.mozilla.org/r/229922/#review235942

Looks good! Please just update the comment before landing.

::: python/mozbuild/mozbuild/backend/tup.py:191
(Diff revision 2)
>  
>          # These are 'group' dependencies - All rules that list these as an output
>          # will be built before any rules that list this as an input.
>          self._installed_idls = '$(MOZ_OBJ_ROOT)/<installed-idls>'
>          self._installed_files = '$(MOZ_OBJ_ROOT)/<installed-files>'
> +        # The preprocessor can hide dependencies from Tup in some cases (when

This comment confused me at first - it's not really that the preprocessor is hiding dependencies from tup, it's that the dependencies on those files aren't specified anywhere in moz.build. Tup does see the dependencies, and complains that it wasn't told about them ahead of time since those two files are generated from other rules.

When I first read it I thought somehow tup wasn't picking up those dependencies and failing to re-build application.ini and such when buildid.h changes.
Attachment #8961121 - Flags: review?(mshal) → review+
Comment hidden (mozreview-request)

Comment 8

Last year
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2fdb62d06d0
Do not re-generate buildid.h for every Tup build. r=mshal

Comment 9

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2fdb62d06d0
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.