Closed Bug 1228444 Opened 4 years ago Closed 4 years ago

Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
and move files without preprocessor directives to FINAL_TARGET_FILES.
Attachment #8692690 - Flags: review?(gps)
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.
Attachment #8692692 - Flags: review?(gps)
Attachment #8692692 - Attachment is obsolete: true
Attachment #8692692 - Flags: review?(gps)
Attachment #8692697 - Flags: review?(gps)
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?
(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 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.
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.
Attachment #8692691 - Attachment is obsolete: true
Attachment #8692691 - Flags: review?(gps)
Attachment #8693122 - Flags: review?(gps)
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.
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.
Blocks: 1228998
Attachment #8692689 - Flags: review?(gps) → review+
Attachment #8692690 - Flags: review?(gps) → review+
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.
Attachment #8692697 - Flags: review?(gps) → review+
Comment on attachment 8693122 [details] [diff] [review]
Make DIST_FILES a HierarchicalStringList, like FINAL_TARGET_FILES

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

Nice.
Attachment #8693122 - Flags: review?(gps) → review+
Depends on: 1229279
Blocks: 1229803
Blocks: 1229810
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.