1256033 - Remove layout/style/test/Makefile.in

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

This one is a little tricky because the generated file depends on a host binary, so we have to add support for that.

Comment 1

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1256033 - Allow GENERATED_FILES scripts to depend on other generated files; r?glandium

If we add an ObjDirPath as an input to a GENERATED_FILES script, we
should run it in the misc tier to ensure the dependent files are created
beforehand. We need to use Path objects instead of raw filenames in the
GeneratedFile object so the recursive make backend can distinguish
between source and objdir files.

Review commit: https://reviewboard.mozilla.org/r/39609/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39609/

Comment 2

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1256033 - Remove layout/style/test/Makefile.in; r?glandium

The css_properties.js rule can be converted into gen-css-properties.py,
which we can install with TEST_HARNESS_FILES instead of the
mochitest.ini manifest.

Review commit: https://reviewboard.mozilla.org/r/39611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39611/

Comment 3

[Michael Shal [:mshal]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd802390c1e

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8729868 [details]
MozReview Request: Bug 1256033 - Allow GENERATED_FILES scripts to depend on other generated files; r?glandium

https://reviewboard.mozilla.org/r/39609/#review36467

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8729869 [details]
MozReview Request: Bug 1256033 - Remove layout/style/test/Makefile.in; r?glandium

https://reviewboard.mozilla.org/r/39611/#review36469

Please preserve the comment about TestCSSPropertyLookup.cpp somehow. Maybe paste it in moz.build as-was, adding "This was in the Makefile and should be restored somehow some day" (specifically, I think it could and should be turned into a gtest)

::: layout/style/test/mochitest.ini
(Diff revision 1)
>    visited_image_loading_frame.html
>    visited_image_loading.sjs
>    visited-lying-inner.html
>    visited-pref-iframe.html
>    xbl_bindings.xml
> -generated-files = css_properties.js

I won't claim to know test manifests much, but I think this ought to stay here (as well as the standalone css_properties.js in the same file)

Comment 6

[(not currently active) Ted Mielczarek]

I think chmanchester could comment on the test manifest bit.

Comment 7

[Michael Shal [:mshal]]

Here's what I tried:

1) Leaving css_properties.js in mochitest.ini without TEST_HARNESS_FILES += css_properties.js in moz.build: 13 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | uncaught exception - ReferenceError: gLonghandProperties is not defined at @http://mochi.test:8888/tests/layout/style/test/test_property_database.html:34:10

2) Adding TEST_HARNESS_FILES += css_properties.js, and keeping mochitest.ini as is: ValueError: Item already in manifest: testing/mochitest/tests/layout/style/test/css_properties.js

3) Adding TEST_HARNESS_FILES += css_properties.js, removing 'css_properties.js' from mochitest.ini, but leaving the 'generated-files = css_properties.js' in mochitest.ini: Error processing test manifest /home/mshal/mozilla-central-git/layout/style/test/mochitest.ini: entry in generated-files not present elsewhere in manifest: css_properties.js

So is what I did in the patch correct? (Adding css_properties.js in TEST_HARNESS_FILES, and remove references to it in mochitest.ini)

Or is there some way to get the test to pass while keeping mochitest.ini the way it is?

Comment 8

[Chris Manchester (limited bugmail, email directly)]

You did the right thing.

generated-files in a *.ini manifest tells the manifest processing in the emitter and backend we don't need to install that file, because some other part of build system has dropped it in its final location, but tests need it, so mention it in the install manifest so it doesn't get deleted when we process the install manifest.

Ideally I guess GENERATED_FILES would let us just say:

GENERATED_FILES += ['!/_tests/testing/mochitest/tests/layout/style/test/css_properties.js']

and we wouldn't need TEST_HARNESS_FILES do that part for us, but I think the inputs to GENERATED_FILES are always interpreted as files in that moz.build's directory.

Comment 9

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Comment on attachment 8729869 [details]
> MozReview Request: Bug 1256033 - Remove layout/style/test/Makefile.in;
> r?glandium
> 
> https://reviewboard.mozilla.org/r/39611/#review36469
> 
> Please preserve the comment about TestCSSPropertyLookup.cpp somehow. Maybe
> paste it in moz.build as-was, adding "This was in the Makefile and should be
> restored somehow some day" (specifically, I think it could and should be
> turned into a gtest)

Done.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d799cc3c6894
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d3b754cbcd

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d799cc3c6894
https://hg.mozilla.org/mozilla-central/rev/c1d3b754cbcd