Closed Bug 952335 Opened 6 years ago Closed 6 years ago

[OS.File] Can't link with lz4 on some platforms

Categories

(Toolkit :: OS.File, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 4 obsolete files)

For some reason, bug 854169 breaks Jetpack tests on MacOS X.

Investigating further, it seems that js-ctypes cannot load library "XUL" when Firefox is launched by the Jetpack test suite (and only when it is launched by the Jetpack test suite).

My understanding is that other test runners and the Firefox launch script (or MacOS X itself when launching Firefox) all somehow customize DYLD_LIBRARY_PATH to include "XUL" but the Jetpack test suite doesn't – see http://dxr.mozilla.org/mozilla-central/search?q=LD_LIBRARY_PATH .

This is rather annoying, as it prevents me from landing bug 854169, which is pretty high-priority, so I would be grateful if someone could look into this.
Summary: Jetpack testrunner seems to mess with the library path → Jetpack testrunner doesn't set the correct the library path on MacOS X
Severity: normal → major
Attached patch Experimental patch, WIP (obsolete) — Splinter Review
Attachment #8350590 - Flags: feedback?(dtownsend+bugmail)
Attached patch Experimental patch, WIP (obsolete) — Splinter Review
This didn't seem to fix the issue. I'm coding blind here, does anyone have ideas?
Attachment #8350590 - Attachment is obsolete: true
Attachment #8350590 - Flags: feedback?(dtownsend+bugmail)
Attachment #8350750 - Flags: feedback?(dtownsend+bugmail)
Attachment #8350750 - Flags: feedback?(avarma)
The correct solution here is probably bug 897370
How does the current mozrunner do this? If we can take the bit of that and include it in our fork of mozrunner then it would mean a faster fix that waiting on bug 897370
Attachment #8350750 - Flags: feedback?(dtownsend+bugmail)
As the lz4 patch is currently blocking work by both Perf and FHR teams, I'll try and find a hack that would make it possible for me to land the lz4 patch without breaking the Jetpack tests. However, even if this hack works, it will be quite unsatisfactory, as it will only delay the breakage.

Dave, could you assign someone to look at the mozrunner issue?
Flags: needinfo?(dtownsend+bugmail)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #5)
> As the lz4 patch is currently blocking work by both Perf and FHR teams, I'll
> try and find a hack that would make it possible for me to land the lz4 patch
> without breaking the Jetpack tests. However, even if this hack works, it
> will be quite unsatisfactory, as it will only delay the breakage.
> 
> Dave, could you assign someone to look at the mozrunner issue?

The reality is that there isn't anyone on my team that knows this stuff really, which is why we're trying to replace it. I can take a look and see if at least the newer mozrunner would fix it and mybe figure out something based on that. Is there a simple way to test this problem?
Flags: needinfo?(dteller)
At the moment, the simplest way is to import patches 1-5 (not 6, which is the temporary workaround) of bug 854169 and try and run the Jetpack tests on MacOS X. I can fold the patches together if you prefer, to avoid any confusion.
Flags: needinfo?(dteller)
I tested ctypes.open(OS.Constants.Path.libxul) in a current nightly started by Firefox's test harness. It indeed failed to load the library. I then tried that in my regular nightly build just started normally. That failed too. Then I tried ctypes.open(OS.Path.join(OS.Constants.Path.libDir, OS.Constants.Path.libxul)). That worked in both cases (and following that attempting to load just XUL worked fine).

Then I built current nightly with your patch 6 backed out. It fails running the Jetpack tests. It also fails just starting from the command line (dist/bin/Nightly.app/Contents/MacOS/firefox) but works when starting by double clicking the app bundle. Changing lz4_internal.js to use the full path for XUL made everything work.

So, I think this isn't just a problem with the Jetpack harness as it affects the command line launch too. I think the right fix is to always use the full path to load XUL with ctypes either by changing OS.Constants.Path.libxul to be the full path or by changing every caller. What do you think?
Flags: needinfo?(dtownsend+bugmail) → needinfo?(dteller)
(In reply to Dave Townsend (:Mossop) from comment #8)
> So, I think this isn't just a problem with the Jetpack harness as it affects
> the command line launch too. I think the right fix is to always use the full
> path to load XUL with ctypes either by changing OS.Constants.Path.libxul to
> be the full path or by changing every caller. What do you think?

I seem to remember that this doesn't work on Android. I suppose I can #ifdef a little bit more the path on OS.Constants.Path, though. I'll give it a try once I have some time.
Flags: needinfo?(dteller)
David, fwiw, the issue I had the other day, when I told you my patch using lz4 was not working, is due to being unable to load libxul, so basically the same issue pointed out in comment 8. Using the whole path in ctypes.open does indeed solve the problem, so this is also an issue in regular usage of the lz4 option, not just a jetpack testrunner issue.
Blocks: 818587
(In reply to Marco Bonardo [:mak] from comment #10)
> David, fwiw, the issue I had the other day, when I told you my patch using
> lz4 was not working, is due to being unable to load libxul, so basically the
> same issue pointed out in comment 8. Using the whole path in ctypes.open
> does indeed solve the problem, so this is also an issue in regular usage of
> the lz4 option, not just a jetpack testrunner issue.

Thanks. Interesting that it doesn't fail the tests.
Mak, what's your platform?
Component: Jetpack SDK → OS.File
Product: Mozilla Labs → Toolkit
Summary: Jetpack testrunner doesn't set the correct the library path on MacOS X → [OS.File] Can't link with lz4 on some platforms
Assignee: nobody → dteller
Attached patch Experimental patch (obsolete) — Splinter Review
Attachment #8350750 - Attachment is obsolete: true
Attachment #8350750 - Flags: feedback?(avarma)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #12)
> Mak, what's your platform?

sorry for late, please needinfo next time. I was on Mac.
Attachment #8362699 - Attachment filename: libxul → Full path for libxul on Mac
Attachment #8362699 - Flags: review?(mh+mozilla)
Comment on attachment 8362699 [details] [diff] [review]
Experimental patch

I'd suggest correcting the comments here
Dave: Sure. Do you have ideas how to test this? Right now, I'm thinking of writing a Jetpack test for a non-Jetpack feature, but I'm not sure if I can do this in the current bug or if I should do it on github once the patch has landed.

Try: https://tbpl.mozilla.org/?tree=Try&rev=2cbeb234b81c
Flags: needinfo?(dtownsend+bugmail)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #16)
> Dave: Sure. Do you have ideas how to test this? Right now, I'm thinking of
> writing a Jetpack test for a non-Jetpack feature, but I'm not sure if I can
> do this in the current bug or if I should do it on github once the patch has
> landed.
> 
> Try: https://tbpl.mozilla.org/?tree=Try&rev=2cbeb234b81c

If an SDK test is the only way to catch this then it sounds like a good idea to me, whatever makes your life easier. If you do it in this bug and land in m-c then we'll just cherry-pick the SDK changes before we do our next uplift.
Flags: needinfo?(dtownsend+bugmail)
Attached patch Fixing OS.Constants.Path.libxul (obsolete) — Splinter Review
Same patch, with a jetpack test.
I have checked that the test fails prior to the patch and succeeds after.
Attachment #8362699 - Attachment is obsolete: true
Attachment #8362699 - Flags: review?(mh+mozilla)
Attachment #8369368 - Flags: review?(dtownsend+bugmail)
Attachment #8369368 - Flags: review?(dtownsend+bugmail) → review+
Added missing commit message.
Attachment #8369368 - Attachment is obsolete: true
Attachment #8369989 - Attachment is patch: true
https://hg.mozilla.org/integration/fx-team/rev/08f51dedf516
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b9b6195901905ec08bcdfd3a3c51bbdd8a9f1f36
Bug 952335 - Provide the full path for OS.Constants.Path.libxul on OSX. r=mossop
https://hg.mozilla.org/mozilla-central/rev/08f51dedf516
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
Comment on attachment 8369989 [details] [diff] [review]
Fixing OS.Constants.Path.libxul

[Approval Request Comment]
Bug caused by (feature/regressing bug #): OS.File lz4
User impact if declined: Better crash reporting in bug 875562 can't be uplifted to 29
Testing completed (on m-c, etc.): It landed recently. Might need a few days.
Risk to taking this patch (and alternatives if risky): Should be low risk
String or IDL/UUID changes made by this patch: None

This approval is tied to bug 875562. If bug 875562 doesn't get uplifted, we can likely cancel this uplift.
Attachment #8369989 - Flags: approval-mozilla-aurora?
Target Milestone: mozilla29 → mozilla30
Attachment #8369989 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.