Closed Bug 934864 Opened 6 years ago Closed 6 years ago

Make PP_TARGETS handle directories in output

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: nalexander, Assigned: glandium)

References

Details

Attachments

(3 files, 2 obsolete files)

At the moment, PP_TARGETS += target with target_FILES += foo/bar.in and baz/nuts.in doesn't write foo/bar and baz/nuts, it writes bar and nuts to the same directory.  It would be better if PP_TARGETS handled directories in output correctly.

One way to do this would be to define target_SOURCE and then have target_FILES be relative to that source.

While we're improving PP_TARGETS, let's have the build system define target_OUTPUTS so that Makefile.in files don't have to declare this themselves.  And while we're making requests, can we make PP_TARGETS handle input dependencies, like makedeps?
(In reply to Nick Alexander :nalexander from comment #0)
> And while we're making requests, can we make PP_TARGETS handle
> input dependencies, like makedeps?

That's a separate issue.
Assignee: nobody → mh+mozilla
Also, refactored them for more debuggability and clarity.

Which, sadly, requires an awful hack because GNU make 3.81 is buggy:

$ cat > test.mk <<EOF
a: b
a:
	@echo $<
a: d
b d:
EOF
$ make -f test.mk
d

GNU make 4.0 returns b, pymake too. And it's what people would expect. Interestingly,
$ cat > test.mk <<EOF
a: b
a: c
a:
	@echo $<
a: d
b c d:
EOF
$ make -f test.mk
b

When there are two dependency rules before the rule with the recipe, $< has the expected value.
Attachment #827374 - Flags: review?(gps)
Comment on attachment 827374 [details] [diff] [review]
Add option to make INSTALL_TARGETS and PP_TARGETS keep the original path when copying/preprocessing

This looks pretty good to me, modulo that there's an implicit $SRCDIR base for all _FILES, which makes installing, say, ../foo/bar awkward.

I think that _KEEP_PATH should be the default (I know the existing behaviour was surprising to me).  It seems so likely that people will get this wrong and be very confused.

I'll use this for an update of Bug 933300 and see how it feels.  Thanks, Mike!
(In reply to Nick Alexander :nalexander from comment #3)
> This looks pretty good to me, modulo that there's an implicit $SRCDIR base
> for all _FILES, which makes installing, say, ../foo/bar awkward.

If you're installing ../foo/bar, you're doing it wrong. You should be installing foo/bar from .. .

> I think that _KEEP_PATH should be the default (I know the existing behaviour
> was surprising to me).  It seems so likely that people will get this wrong
> and be very confused.

That would require finding and changing all existing occurrences relying on the current behaviour, and i'm sure there are many.
This could be folded in the other patch when landing. I'm not entirely sold to this yet, but it did help me locally when debugging bug 935305, and it raises awareness to all those things we do that make -s doesn't tell. Maybe that will push us to move that to manifests.
Attachment #827820 - Flags: review?(gps)
The output is maybe less confusing with an accompanying change to REPORT_BUILD itself.
Attachment #827837 - Flags: review?(gps)
Attachment #827820 - Attachment is obsolete: true
Attachment #827820 - Flags: review?(gps)
With js/src/config/rules.mk sync.
Attachment #827842 - Flags: review?(gps)
Attachment #827837 - Attachment is obsolete: true
Attachment #827837 - Flags: review?(gps)
Could we trim trailing slashes for _PATH?  It matters for dependencies, i.e. foo//bar gets generated but the dep is on foo/bar, so you can get races.
In my use case, I preprocess foo/bar.in into baz/foo/bar, and the existing REPORT_BUILD doesn't work since there's no $DEPTH in my $@.  Perhaps we want an abspath around $@?
Final comment: my proposed use for INSTALL_TARGETS is installing the icon files in m/a/b.  Those come from the branding directory.  So I can either have 4 INSTALL_TARGETS in m/a/b/Maekfile.in, or install those files from the branding directories Makefile.in.  Writing files into res outside of m/a/b/Makefile.in is a nightmare (witness strings.xml), so I'd *really* rather not do that.
Comment on attachment 827374 [details] [diff] [review]
Add option to make INSTALL_TARGETS and PP_TARGETS keep the original path when copying/preprocessing

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

This is insanely difficult to read. But, it's necessary. I wouldn't be surprised if there were a bug somewhere. But, I would think bugs would surface pretty quickly via obvious build failures.
Attachment #827374 - Flags: review?(gps) → review+
Comment on attachment 827842 [details] [diff] [review]
Add REPORT_BUILD do INSTALL_TARGETS and PP_TARGETS

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

This will add more noise, but I suppose that is good: it will help us target what to prioritize for install manifest conversion.
Attachment #827842 - Flags: review?(gps) → review+
Blocks: 935305
This addresses comment 8.
Attachment #828973 - Flags: review?(gps)
These patches are working well for me: http://tbpl.mozilla.org/?tree=Try&rev=2df93df8a97d

I did have to make my deps absolute path -- so if I used PP_TARGETS to install one file into $CURDIR, I needed to give $(CURDIR)/out instead of the old out.  This could be a VPATH issue, but it was a change.
Attachment #828973 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/97daebf6405b
https://hg.mozilla.org/mozilla-central/rev/4810e8f4eb98
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Backed out the REPORT_BUILD part for causing bug 937332. Wasn't important for this bug anyways.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad04a01e825
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.