provide moz.build syntax for generated exported headers

RESOLVED FIXED in Firefox 45

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: froydnj, Assigned: ted)

Tracking

(Blocks: 1 bug)

unspecified
mozilla45
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

We have GENERATED_FILES, but we still wind up having to write INSTALL_TARGETS bits for any headers that we generate.  It would be better if moz.build wrote out the necessary INSTALL_TARGETS bits.

Suggested syntax:

EXPORTS += [
    '!GeneratedHeader.h',
]

since a '!' prefix means things in the objdir.
We might not even need the '!' prefix, since presumably mozbuild already knows if the file is in GENERATED_FILES. But being consistent with other variables doesn't hurt.
(Assignee)

Comment 2

3 years ago
My original thought when I was writing the GENERATED_FILES stuff was that you'd do like
g = GeneratedFile('foo.h')

EXPORTS += [g]

...but having the moz.build reader deduce this from the filenames seems fine too, just like:
GENERATED_FILES += ['foo.h']
EXPORTS += ['foo.h'] # make this work
(Assignee)

Comment 3

3 years ago
For prior art here, I was toying around with Bazel last week and wrote a bazel BUILD file for rr:
https://github.com/luser/rr/blob/bazel/BUILD

You'll note there the GENERATED_FILES (that variable name isn't important) get listed in the cc_binary(srcs=...) list, and the rule (that I wrote) that generates them simply states that these filenames are outputs:
https://github.com/luser/rr/blob/bazel/gen_syscalls.bzl#L15

and bazel figures the rest out.
The deduction approach seems better, especially since experience with the current incarnation of GENERATED_FILES says that GENERATED_FILES is clunky.  It really should be something like:

with GeneratedFile('foo'):
    SCRIPT = ...
    ENTRY_POINT = ...
    INPUTS = ....
(Assignee)

Updated

2 years ago
Assignee: nobody → ted
(Assignee)

Updated

2 years ago
Depends on: 1229279
(Assignee)

Comment 5

2 years ago
Created attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

This change allows specifying objdir-relative paths in EXPORTS to enable
exporting entries from GENERATED_FILES. Objdir paths in EXPORTS that are
not in GENERATED_FILE will raise an exception.

Example:
```
EXPORTS += ['!g.h', 'f.h']
GENERATED_FILES += ['g.h']
```
Attachment #8694209 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

2 years ago
I didn't go through the entire tree and fix every instance of GENERATED_FILES possible. It should be fairly straightforward, if you look at the patch you can see that I fixed one instance in accessible/xpcom. It should just be a matter of adding !foo.h to EXPORTS in any dir where foo.h is in GENERATED_FILES and removing the corresponding INSTALL_TARGETS bits from the Makefile.in.
Attachment #8694209 - Flags: review?(mh+mozilla)
Comment on attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

https://reviewboard.mozilla.org/r/26705/#review24281

I'd rather avoid the fragmentation and do the following:
- Modify the mozbuild.frontend.data.Exports class to be like the TestingFiles one (i.e. a FinalTargetFiles derivative with a specific overridden install_target).
- Have the Exports objects be generated from the loop that handles FINAL_TARGET{,_PP}_FILES and TESTING_FILES in emitter.py, and handle the GENERATED_FILES check there too. This will have the magic effect of making the right thing happen whenever we add !paths to e.g. FINAL_TARGET_FILES in the future, which we will undoubtedly, as that would allow to move some more stuff from moz.build.
- The above means recursivemake would treat those Exports in _process_final_target_files, which would get the code to add INSTALL_TARGETS as well.

Note: I haven't looked at the tests.
(Assignee)

Comment 9

2 years ago
OK, I will take a look at making those changes.
(Assignee)

Comment 10

2 years ago
Comment on attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26705/diff/1-2/
Attachment #8694209 - Flags: review?(mh+mozilla)
Comment on attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

https://reviewboard.mozilla.org/r/26705/#review24527

::: python/mozbuild/mozbuild/backend/recursivemake.py:1292
(Diff revision 2)
> -        if target.startswith('dist/bin'):
> -            install_manifest = self._install_manifests['dist_bin']
> -            reltarget = mozpath.relpath(target, 'dist/bin')
> -        elif target.startswith('dist/xpi-stage'):
> -            install_manifest = self._install_manifests['dist_xpi-stage']
> -            reltarget = mozpath.relpath(target, 'dist/xpi-stage')
> +        for (path, manifest) in (
> +                ('dist/bin', 'dist_bin'),
> +                ('dist/xpi-stage', 'dist_xpi-stage'),
> +                ('_tests', 'tests'),
> +                ('dist/include', 'dist_include'),
> +                ):

Note, if the _tests install_manifest was _tests, we could use path.replace('/', '_'). That's been bugging me for a while... Followup?

::: python/mozbuild/mozbuild/backend/recursivemake.py:1310
(Diff revision 2)
> +                if isinstance(f, SourcePath):

You want this branch to handle AbsolutePaths as well, so you might as well use not isinstance(f, ObjDirPath)

::: python/mozbuild/mozbuild/frontend/emitter.py:675
(Diff revision 2)
> +                    if isinstance(f, SourcePath):

Same as previous comment, you want to cover AbsolutePaths here too.
Attachment #8694209 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

https://reviewboard.mozilla.org/r/26705/#review24533

::: python/mozbuild/mozbuild/frontend/emitter.py:675
(Diff revision 2)
> +                    if isinstance(f, SourcePath):

Oh, just thought of something else: we should probably reject non SourcePaths for FinalTargetPreprocessedFiles.
Attachment #8694209 - Flags: review+
Comment on attachment 8694209 [details]
MozReview Request: bug 1160185 - support GENERATED_FILES in EXPORTS. r?glandium

I didn't mean for the r+ to be removed.
Attachment #8694209 - Flags: review+
(Assignee)

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/26705/#review24533

> Oh, just thought of something else: we should probably reject non SourcePaths for FinalTargetPreprocessedFiles.

Added a check + exception for this and a test for it.
(Assignee)

Comment 15

2 years ago
https://reviewboard.mozilla.org/r/26705/#review24527

> Note, if the _tests install_manifest was _tests, we could use path.replace('/', '_'). That's been bugging me for a while... Followup?

Good call! That wasn't hard to fix so I just did it.
(Assignee)

Updated

2 years ago
Blocks: 1230750

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ca0a1e37264
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.