dependencies not handled correctly for generating nsCSSPropsGenerated.inc (needed changes for bug 1294660 not detected, needed clobber)

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: aryx, Assigned: mshal)

Tracking

(Blocks 1 bug)

Trunk
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

When bug 1294660 landed, the build failed, but a clobber fixed it: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1348afa1c6a0f2fd30db419f7099334e965f4ddb&filter-searchStr=10.7&selectedJob=34569644

It had been backed out in the meantime and eventually relanded with a modified CLOBBER file.

Ryan told me to file a bug, please tell me if you need more information.
Summary: needed changes for bug 1294660 not detected, needed clobber → dependencies not handled correctly for generating nsCSSPropsGenerated.inc (needed changes for bug 1294660 not detected, needed clobber)
Does the code at:
http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/layout/style/moz.build#251
imply that the #include dependencies of PythonCSSProps.h (i.e., nsCSSPropList.h), or does that need to be listed explicitly?
Flags: needinfo?(mshal)
See Also: → 1272488, 1284169
Er, I guess the issue was that the ACDEFINES changed.
(Assignee)

Comment 3

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #1)
> Does the code at:
> http://searchfox.org/mozilla-central/rev/
> b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/layout/style/moz.build#251
> imply that the #include dependencies of PythonCSSProps.h (i.e.,
> nsCSSPropList.h), or does that need to be listed explicitly?

We have bug 1281614 to try to make that more automatic, but for now we specify those include dependencies manually in the Makefile: https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/layout/style/Makefile.in#7

> Er, I guess the issue was that the ACDEFINES changed.

Ahh, thanks for figuring that out! I can reproduce it by doing the following:

$ ./mach build
# uncomment MOZ_ENABLE_MASK_AS_SHORTHAND=1 in old-configure.in
$ ./mach configure
$ cd objdir
$ make mozilla-config.h
$ cd layout/style
$ make

(A second mach build after enabling MOZ_ENABLE_MASK_AS_SHORTHAND would also work, but that would rebuild everything...)

I think the right thing to do here is to provide a way for buildconfig.py to say that scripts depend on config.status. The forthcoming patch does so, but it's still fairly manual.
Flags: needinfo?(mshal)
(Assignee)

Updated

3 years ago
Assignee: nobody → mshal
Comment hidden (mozreview-request)

Comment 5

3 years ago
mozreview-review
Comment on attachment 8784943 [details]
Bug 1297718 - Add config.status to sys.modules for dependency detection;

https://reviewboard.mozilla.org/r/74270/#review72140

I'm assuming there is magic that takes the return values for these "generate" functions and turns them into dependencies. Otherwise you forgot to upload part of this commit :)

::: python/mozbuild/mozbuild/base.py:201
(Diff revision 1)
> +        if self._config_status is None:
> +            self._config_status = os.path.join(self.topobjdir, 'config.status')
> +        return self._config_status

We don't need a cache here. The caching is for things that cost a bit to resolve. Since self.topobjdir is itself cached, just do the os.path.join() at call time, every time.
Attachment #8784943 - Flags: review?(gps) → review+
(Assignee)

Comment 6

3 years ago
mozreview-review-reply
Comment on attachment 8784943 [details]
Bug 1297718 - Add config.status to sys.modules for dependency detection;

https://reviewboard.mozilla.org/r/74270/#review72140

Yep, that was added in bug 1215526. The action can return a set() to indicate success, and the files in the set are added as dependencies to the .pp file.

Comment 7

3 years ago
mozreview-review
Comment on attachment 8784943 [details]
Bug 1297718 - Add config.status to sys.modules for dependency detection;

https://reviewboard.mozilla.org/r/74270/#review72168

::: dom/bindings/GenerateCSS2PropertiesWebIDL.py:75
(Diff revision 1)
>      idlTemplate = idlFile.read()
>      idlFile.close()
>  
>      output.write("/* THIS IS AN AUTOGENERATED FILE.  DO NOT EDIT */\n\n" +
>                   string.Template(idlTemplate).substitute({"props": props}) + '\n')
> +    return set(buildconfig.deps)

Relying on every script using buildconfig to do this manually is bound to fail. Plus, that doesn't cover the other ways config.status can be used by scripts without buildconfig (using MozbuildObject.from_environment directly).

Instead, I'd change mozbuild.backend.configenvironment.BuildConfig.from_config_status in a way that makes config.status seen by mozbuild.pythonutil.iter_modules_in_path
Attachment #8784943 - Flags: review-
(Assignee)

Updated

3 years ago
Blocks: clobber
(Assignee)

Comment 8

3 years ago
Adding config.status to sys.modules manually in from_config_status() seems to do the trick. However, I noticed that we weren't writing out dependencies if the GENERATED_FILES function returned a set, and returning an empty set doesn't work. I changed this to allow both None and an empty set as "success" values, and it will write out the python module dependencies in all cases.
Comment hidden (mozreview-request)

Comment 10

3 years ago
mozreview-review
Comment on attachment 8784943 [details]
Bug 1297718 - Add config.status to sys.modules for dependency detection;

https://reviewboard.mozilla.org/r/74270/#review73444

::: python/mozbuild/mozbuild/action/file_generate.py:67
(Diff revision 2)
> -            if isinstance(ret, set) and ret:
> -                ret |= set(iter_modules_in_path(buildconfig.topsrcdir,
> +            if isinstance(ret, set):
> +                deps = ret
> +                # The script succeeded, so reset |ret| to indicate that.
> +                ret = None
> +            else:
> +                deps = set()

I think it would be better to still not write out any deps when the command fails. So just removing the `and ret` should be enough.
Attachment #8784943 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

3 years ago
(In reply to Mike Hommey [:glandium] from comment #10)
> I think it would be better to still not write out any deps when the command
> fails. So just removing the `and ret` should be enough.

Not writing out deps when the command fails makes sense. However, I think just removing the 'and ret' would mean that we don't write out any deps if the command doesn't return anything, or if it returns 0 to indicate success. I've changed it to only write out deps "if not ret" after we check if ret is a set, since ret is cleared if it is.
Comment hidden (mozreview-request)

Comment 13

3 years ago
mozreview-review
Comment on attachment 8784943 [details]
Bug 1297718 - Add config.status to sys.modules for dependency detection;

https://reviewboard.mozilla.org/r/74270/#review73814

::: python/mozbuild/mozbuild/action/file_generate.py:64
(Diff revision 3)
>              # We treat sets as a statement of success.  Everything else
>              # is an error (so scripts can conveniently |return 1| or
>              # similar).

Can you update this comment to reflect the fact that 0 is an explicit success too?
Attachment #8784943 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 14

3 years ago
(In reply to Mike Hommey [:glandium] from comment #13)
> ::: python/mozbuild/mozbuild/action/file_generate.py:64
> (Diff revision 3)
> >              # We treat sets as a statement of success.  Everything else
> >              # is an error (so scripts can conveniently |return 1| or
> >              # similar).
> 
> Can you update this comment to reflect the fact that 0 is an explicit
> success too?

Ok, I clarified the comments a bit more.

Comment 15

3 years ago
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bacde849ce
Add config.status to sys.modules for dependency detection; r=glandium

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2bacde849ce
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.