1228444 - Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

DIST_FILES is essentially meant to be like FINAL_TARGET_FILES, with preprocessing. It however currently only support things directly under FINAL_TARGET, which is unnecessarily limiting.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Validate that FINAL_TARGET_FILES source files exist

Comment 2

[Mike Hommey [:glandium]]

Attachment: Don't silence "no preprocessor directives found" warnings for DIST_FILES

and move files without preprocessor directives to FINAL_TARGET_FILES.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

Comment 4

[Mike Hommey [:glandium]]

Attachment: Rename DIST_FILES to FINAL_TARGET_PP_FILES

This makes it clearer that really it's the same thing as FINAL_TARGET,
with preprocessing.
We still keep DIST_FILES in backend.mk because it's shorter and doesn't
really matter.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Rename DIST_FILES to FINAL_TARGET_PP_FILES

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8692690 [details] [diff] [review]
Don't silence "no preprocessor directives found" warnings for DIST_FILES

Review of attachment 8692690 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ -1271,5 @@
>  ifneq ($(DIST_FILES),)
>  DIST_FILES_PATH := $(FINAL_TARGET)
> -# We preprocess these, but they don't necessarily have preprocessor directives,
> -# so tell them preprocessor to not complain about that.
> -DIST_FILES_FLAGS := --silence-missing-directive-warnings

If we're going to do this, could we possibly yell at people to move their no-directives files to a different variable, so we don't have the inevitable pointless warnings creeping back in over time?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] (high latency through 29 November) from comment #6)
> If we're going to do this, could we possibly yell at people to move their
> no-directives files to a different variable, so we don't have the inevitable
> pointless warnings creeping back in over time?

Fair enough, filed bug 1228467.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8692690 [details] [diff] [review]
Don't silence "no preprocessor directives found" warnings for DIST_FILES

Review of attachment 8692690 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/extensions/bootstrap/moz.build
@@ +11,5 @@
>      'worker.js',
>  ]
> +
> +FINAL_TARGET_FILES += [
> +    'bootstrap.js',

worker.js also needs to move here. Fixed locally. Not refreshing patches just for that.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

The (sub) paths were missing in the recursive make backend. This should have been caught by tests, but I don't think the backend unit tests are a very useful form for these things.

I'm planning to write an actual testcase in the form of a growing pseudo-project, which will validate that a set of moz.build files going through different backends (Recursive and Faster), running make, does generate an expected set of files in objdir/dist.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8693122 [details] [diff] [review]
Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

Review of attachment 8693122 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +674,5 @@
>  
>          expected = [
> +            'DIST_FILES_0 += install.rdf',
> +            'DIST_FILES_0 += main.js',
> +            'DIST_FILES_0_PATH := $(DEPTH)/dist/bin',

need a / at the end here.

Comment 11

[(not currently active) Ted Mielczarek]

Nice, I noticed this while porting things to moz.build and it was annoying. I would love to get to the point where we could just merge these two variables, but that does add complexity since one of them is handled with install manifests and one of them is handled with INSTALL_TARGETS.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8692697 [details] [diff] [review]
Rename DIST_FILES to FINAL_TARGET_PP_FILES

Review of attachment 8692697 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +41,5 @@
>      ConfigFileSubstitution,
>      ContextDerived,
>      ContextWrapped,
>      Defines,
> +    FinalTargetPreprocessedFiles,

Please preserve sorted order.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +32,5 @@
>      ChromeManifestEntry,
>      ConfigFileSubstitution,
>      ContextWrapped,
>      Defines,
> +    FinalTargetPreprocessedFiles,

Please preserve sorted order.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +14,5 @@
>      BrandingFiles,
>      ChromeManifestEntry,
>      ConfigFileSubstitution,
>      Defines,
> +    FinalTargetPreprocessedFiles,

Please preserve sorted order.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8693122 [details] [diff] [review]
Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

Review of attachment 8693122 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5730e38da61
https://hg.mozilla.org/integration/mozilla-inbound/rev/78999cfd9106
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94d5b12e097
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4fc89f59116

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/504865992b1a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5730e38da61
https://hg.mozilla.org/mozilla-central/rev/78999cfd9106
https://hg.mozilla.org/mozilla-central/rev/f94d5b12e097
https://hg.mozilla.org/mozilla-central/rev/c4fc89f59116
https://hg.mozilla.org/mozilla-central/rev/504865992b1a