Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Reporter
	Description • 8 years ago

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.

	David Baron :dbaron:
	Comment 1 • 8 years ago

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?

	David Baron :dbaron:
	Comment 2 • 8 years ago

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

	Michael Shal [:mshal] Assignee
	Comment 3 • 8 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.

	Gregory Szorc [:gps]
	Comment 5 • 8 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.

	Michael Shal [:mshal] Assignee
	Comment 6 • 8 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.

	Mike Hommey [:glandium]
	Comment 7 • 8 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

	Michael Shal [:mshal] Assignee
	Comment 8 • 8 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.

	Mike Hommey [:glandium]
	Comment 10 • 8 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.

	Michael Shal [:mshal] Assignee
	Comment 11 • 8 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.

	Mike Hommey [:glandium]
	Comment 13 • 8 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?

	Michael Shal [:mshal] Assignee
	Comment 14 • 8 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.

	Pulsebot
	Comment 15 • 8 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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c2bacde849ce