Port xpcom/tests INSTALL_TARGETS to moz.build

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

unspecified
mozilla47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8711110 [details] [diff] [review]
0003-Bug-1241976-port-INSTALL_TARGETS-in-xpcom-tests-to-m.patch

We can use the FINAL_TARGET trick here to get things installed in the right place. Is there a way to get rid of the remaining LDFLAGS filtering in component_no_aslr/Makefile.in?
Attachment #8711110 - Flags: review?(ted)
Comment on attachment 8711110 [details] [diff] [review]
0003-Bug-1241976-port-INSTALL_TARGETS-in-xpcom-tests-to-m.patch

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

I love simple patches that remove whole Makefiles. :)
Attachment #8711110 - Flags: review?(ted) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8714823 [details] [diff] [review]
0001-Bug-1241976-port-INSTALL_TARGETS-in-xpcom-tests-to-m.patch

Unfortunately this patch breaks xpcshell tests. I thought I verified that the files were being installed in the same place, but apparently not. The Makefile bits install the files directly into objdir/_tests/xpcshell/xpcom/tests/unit, but the moz.build equivalent installed them into objdir/_tests/xpcshell/xpcom/tests/unit/components (with the extra '/components' on the end), so the tests can't find them. This happens because XPCOMBinaryComponent() sets IS_COMPONENT=True, and target_binaries.mk appends the '/components' string based on that flag.

Changing XPCOMBinaryComponent to GeckoSharedLibrary effectively just removes IS_COMPONENT=True so that the libraries get installed in the correct location, and xpcshell passes. However, I'm not sure if that's the proper solution here. Thoughts?
Attachment #8711110 - Attachment is obsolete: true
Attachment #8714823 - Flags: review?(ted)
IS_COMPONENT=True adds things to LDFLAGS, so that's required-ish to build an actual component. -ish because it might just work without, and for test components that might not be a big deal. That being said, there's value in identifying what are meant to be xpcom component from moz.build, so the distinction between XPCOMBinaryComponent and GeckoSharedLibrary has its significance.

How about moving the FINAL_TARGET setting in the XPCOMBinaryComponent template, and override that somehow in the relevant moz.builds?
(Assignee)

Comment 5

3 years ago
Comment on attachment 8714823 [details] [diff] [review]
0001-Bug-1241976-port-INSTALL_TARGETS-in-xpcom-tests-to-m.patch

I'll try glandium's suggestion and either post a new patch or re-ask for review on this one.
Attachment #8714823 - Flags: review?(ted)
(Assignee)

Comment 6

3 years ago
Created attachment 8717666 [details] [diff] [review]
0001-Bug-1241976-port-INSTALL_TARGETS-in-xpcom-tests-to-m.patch

I tried to go with setting FINAL_TARGET in XPCOMBinaryComponent, but mozbuild doesn't seem to like setting/appending anything to it (or even doing print(FINAL_TARGET)). Unless I'm misunderstanding your suggestion, I think to make it work we'd either have to relax the restrictions on FINAL_TARGET, or use some other variable to determine whether or not '/components' is appended. Since this is only for a few tests files, it seems silly to add another mozbuild feature especially for that.

What about this approach? This moves everything into the components/ directory, and just references components/foo.manifest in the js test files. This makes the xpcom/tests moz.build files more similar to js/xpconnect/tests/components/native/moz.build
Attachment #8714823 - Attachment is obsolete: true
Attachment #8717666 - Flags: review?(mh+mozilla)
Attachment #8717666 - Flags: review?(mh+mozilla) → review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdccd4f00208
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

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