The default bug view has changed. See this FAQ.

Make it easier to link to libxul with js-ctypes

RESOLVED FIXED in mozilla16

Status

()

Core
js-ctypes
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

4.88 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.37 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
In some cases, the simplest way of exporting new C functions for use with js-ctypes is to add these functions to libxul and link to libxul with js-ctypes. Unfortunately, for some reason, linking to libxul with js-ctypes is non-trivial/not possible on some platforms – right now, I am thinking of MacOS X from a Chrome Worker.

I suggest making the process simple for end-users. I believe that we could tweak the current API so that calling |ctypes.open()| opens libxul.
I suspect that the easiest would be to publish the location of libxul in OS.Constants.
Created attachment 635708 [details] [diff] [review]
Exposing path to libxul in OS.Constants
Assignee: nobody → dteller
Attachment #635708 - Flags: feedback?(landry)
Created attachment 635709 [details] [diff] [review]
Companion testsuite
Created attachment 635723 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Same one, with a trivial typo fixed and a few additional comments.
Gaston, if you have some time, could you check whether this works on *BSD?
Attachment #635708 - Attachment is obsolete: true
Attachment #635708 - Flags: feedback?(landry)
Attachment #635723 - Flags: review?(khuey)
Attachment #635723 - Flags: feedback?(landry)
Attachment #635709 - Flags: review?(khuey)
You're going to hate me.. but your diff fails to compile with :



/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error: invalid conversion from 'const char*' to 'PRUnichar'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error:   initializing argument 1 of 'void nsAString_internal::Append(
PRUnichar)'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error: invalid conversion from 'const char*' to 'PRUnichar'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error:   initializing argument 1 of 'void nsAString_internal::Append(
PRUnichar)'

Trying with DLL_PREFIX/SUFFIX wrapped without NS_LITERAL_STRING...
err, within, of course.


+    xulName.Append(NS_LITERAL_STRING(DLL_PREFIX));
+    xulName.Append(NS_LITERAL_STRING("xul"));
+    xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX));

builds here, will do the actual testing once it finishes
1 INFO TEST-START | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul
2 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Starting test
3 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Chrome worker created
4 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Test in progress
5 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_xul: opened libxul successfully
6 INFO TEST-END | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | finished in 282ms
7 INFO TEST-START | Shutdown
8 INFO Passed: 4

So with NS_LITERAL_STRING(), all good for me
Attachment #635723 - Flags: feedback?(landry) → feedback+
Created attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Thanks for the BSD test, gaston.
You are, of course, right about the NS_LITERAL_STRING. I had forgotten to update the patch after testing it under Windows.
Attachment #635723 - Attachment is obsolete: true
Attachment #635723 - Flags: review?(khuey)
Attachment #636063 - Flags: review?(khuey)
Comment on attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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

NS_GetSpecialDirectory is not thread-safe.
Attachment #636063 - Flags: review?(khuey) → review-
Comment on attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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

::: dom/system/OSFileConstants.cpp
@@ +412,5 @@
> +    // On other platforms, libxul is a library "xul" with regular
> +    // library prefix/suffix
> +    xulName.Append(NS_LITERAL_STRING(DLL_PREFIX));
> +    xulName.Append(NS_LITERAL_STRING("xul"));
> +    xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX));

You probably want xulName.Append(NS_LITERAL_STRING(DLL_PREFIX "xul" DLL_SUFFIX));
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> Comment on attachment 636063 [details] [diff] [review]
> Exposing path to libxul in OS.Constants
> 
> Review of attachment 636063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> NS_GetSpecialDirectory is not thread-safe.

Great :/

Ok, so plan B is the following:
- extract path to libxpcom during initialization on the main thread;
- re-publish this information as a global string;
- use that global string during initialization of OS.Constants.Sys;
- cry a river for all the other constants I will want to expose to OS.File.
OS: Mac OS X → All
Created attachment 637120 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Attaching an updated version. As suggested by khuey, it extracts the path name from the main thread, during initialization of the ChromeWorker service, and uses this information once chrome workers are effectively created.
Attachment #636063 - Attachment is obsolete: true
Attachment #637120 - Flags: review?(khuey)
Comment on attachment 637120 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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

I'm not going to r- it this time, but I would like to see it again before giving r+.

::: dom/system/OSFileConstants.cpp
@@ +48,5 @@
> +
> +/**
> + * The name of the directory holding all the libraries (libxpcom, libnss, etc.)
> + */
> +nsString gLibDirectory;

Place these global variables in an anonymous namespace please.

@@ +65,5 @@
> +  gInitialized = true;
> +
> +  // Initialize gLibDirectory
> +   nsCOMPtr<nsIFile> xpcomLib;
> +   nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib));

Please fix your indentation in this function.

@@ +67,5 @@
> +  // Initialize gLibDirectory
> +   nsCOMPtr<nsIFile> xpcomLib;
> +   nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib));
> +   if (!NS_SUCCEEDED(rv) || !xpcomLib) {
> +     return false;

Do not return false from a function that has an nsresult retval.

@@ +73,5 @@
> +
> +   nsCOMPtr<nsIFile> libDir;
> +   rv = xpcomLib->GetParent(getter_AddRefs(libDir));
> +   if (!NS_SUCCEEDED(rv)) {
> +     return false;

Do not return false from a function that has an nsresult retval.

@@ +114,5 @@
>  #define S_IRUSR 0400
>  #define S_IRWXU 0700
>  #endif // !defined(S_IRGRP)
>  
> +

Unnecessary newline addition.

@@ +428,5 @@
> +#endif // defined(XP_MACOSX)
> +
> +    JSString* strPathToLibXUL = JS_NewUCStringCopyZ(cx, xulPath.get());
> +    jsval valXul = STRING_TO_JSVAL(strPathToLibXUL);
> +    JS_SetProperty(cx, objSys, "libxulpath", &valXul);

Why don't you care about the return value here.

::: dom/workers/RuntimeService.cpp
@@ +931,5 @@
>                                       mSystemCharset);
>    }
>  
> +  rv = InitOSFileConstants();
> +  if(!NS_SUCCEEDED(rv)) {

Nit: if (
Attachment #637120 - Flags: review?(khuey)
Created attachment 637143 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Thanks for the quick review. Applied your changes.
Attachment #637120 - Attachment is obsolete: true
Attachment #637143 - Flags: review?(khuey)
Comment on attachment 637143 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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

::: dom/system/OSFileConstants.cpp
@@ +46,5 @@
> +
> +/**
> + * The name of the directory holding all the libraries (libxpcom, libnss, etc.)
> + */
> +nsString gLibDirectory;

This isn't an anonymous namespace.  Please put them in a "namespace { };" block so that if anyone does "extern gInitialized" in the future it won't latch on to this.

::: dom/workers/RuntimeService.cpp
@@ +931,5 @@
>                                       mSystemCharset);
>    }
>  
> +  rv = InitOSFileConstants();
> +  if (!NS_SUCCEEDED(rv)) {

if (NS_FAILED(rv))
Attachment #637143 - Flags: review?(khuey) → review+
Created attachment 637146 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Thanks, I just learnt something.
Attachment #637143 - Attachment is obsolete: true
Attachment #637146 - Flags: review+
Comment on attachment 635709 [details] [diff] [review]
Companion testsuite

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

I didn't review the actual test, just the build files.

::: dom/system/tests/Makefile.in
@@ +4,5 @@
> +VPATH            = @srcdir@
> +relativesrcdir   = dom/system/tests
> +
> +MODULE           = test_domsystem
> +DIRS = mochi

You don't need an entire subdirectory for mochitests.  Move the contents of ./mochi/ to this directory.

::: dom/system/tests/mochi/Makefile.in
@@ +7,5 @@
> +MODULE          = domsystem
> +_CHROME_TEST_FILES = \
> +	test_constants.xul \
> +	worker_constants.js \
> +         $(NULL)

No tabs necessary outside of rules.

@@ +9,5 @@
> +	test_constants.xul \
> +	worker_constants.js \
> +         $(NULL)
> +
> + include $(topsrcdir)/config/rules.mk

Nit: extra space?
Attachment #635709 - Flags: review?(khuey) → review+
Created attachment 637157 [details] [diff] [review]
Companion testsuite

Fixed, thanks - and thanks for the quick reviews.
Attachment #635709 - Attachment is obsolete: true
Attachment #637157 - Flags: review+
Created attachment 637183 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Same patch, except we heap-allocate the string instead of static-allocating it, to avoid any leak.
Attachment #637146 - Attachment is obsolete: true
Attachment #637183 - Flags: review+
Created attachment 637670 [details] [diff] [review]
Exposing path to libxul in OS.Constants

+ merging
Attachment #637670 - Flags: review+
Keywords: checkin-needed
Attachment #637183 - Attachment is obsolete: true
Blocks: 552551
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4f69c478fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9924488dd2
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/6b4f69c478fa
https://hg.mozilla.org/mozilla-central/rev/5d9924488dd2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 771087
Blocks: 771928
You need to log in before you can comment on or make changes to this bug.