Closed Bug 1256033 Opened 6 years ago Closed 5 years ago

Remove layout/style/test/Makefile.in

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This one is a little tricky because the generated file depends on a host binary, so we have to add support for that.
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/
Attachment #8729868 - Flags: review?(mh+mozilla)
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/
Attachment #8729869 - Flags: review?(mh+mozilla)
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
Attachment #8729868 - Flags: review?(mh+mozilla) → review+
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)
Attachment #8729869 - Flags: review?(mh+mozilla) → review+
I think chmanchester could comment on the test manifest bit.
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?
Flags: needinfo?(cmanchester)
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.
Flags: needinfo?(cmanchester)
(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.
https://hg.mozilla.org/mozilla-central/rev/d799cc3c6894
https://hg.mozilla.org/mozilla-central/rev/c1d3b754cbcd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.