This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 186000

Summary: Native library lookup is unusable for multiplatform module.
Product: platform Reporter: Petr Nejedly <pnejedly>
Component: Module SystemAssignee: Petr Nejedly <pnejedly>
Status: RESOLVED FIXED    
Severity: normal CC: apireviews, ivan, jglick, jlahoda, jtulach, saubrecht
Priority: P2 Keywords: API, API_REVIEW_FAST
Version: 6.x   
Hardware: All   
OS: All   
URL: http://platform.netbeans.org/articles/installation.html#logical
Issue Type: DEFECT Exception Reporter:
Attachments: Proposed patch
Updated patch
A candidate for a final patch

Description Petr Nejedly 2010-05-12 12:11:27 UTC
Created attachment 98843 [details]
Proposed patch

Current NetBeans module system doesn't effectively allow creating a module using a native library that would work on all supported platforms. All the native libraries have to be located in $cluster/modules/lib, which present a name clash problem when more platforms are to be supported.

Even splitting the module to several implementations, using tokens and OS dependency doesn't solve this problem as there are only OS tokens, no architecture tokens, so you can't have one module for amd64 and another for i386.

Other remaining possibility, calling loadLibrary() with different name on different platforms/architectures breaks for 3rd party libraries, where you have no chance of intercepting/changing the loadLibrary argument.

The last remaining possibility, precopying the right library from somewhere to the lib folder on every start (user might switch between 32 and 64bit JVM easily) is cumbersome.

There is existing proposal to solve this issue inside the module system API docs, namely: http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/api.html#jni

I have implemented this proposal, and as I consider the current state a P2 bug, I propose applying it for 6.9. I have also added a test and bumped the module version, though I'm not sure how to mark this (non-API) addition in a dependable way.
Comment 1 Jaroslav Tulach 2010-05-12 14:24:05 UTC
Y01 The "installation proposal" suggests the proper location of native libraries to be relative to root of clusters. E.g. not cluster/modules/lib/something but cluster/lib/something. The motivation is to separate files that would be placed in /usr/lib and those to be placed in /usr/share directory trees. I'd like to use this opportunity to align with that proposal and recognize the native libraries primarily there. See http://platform.netbeans.org/articles/installation.html

Y02 Make sure you don't increase the number of disk touches during start. There is cache with list of all existing files, maybe you want to use it.

PS: This is an API change, so I am suggesting you to use the API review process.
Comment 2 Jesse Glick 2010-05-12 14:54:53 UTC
(In reply to comment #0)
> calling loadLibrary() with different name on
> different platforms/architectures breaks for 3rd party libraries, where you
> have no chance of intercepting/changing the loadLibrary argument.

The recommended solution is to fix those libraries to call loadLibrary with distinctive names according to platform. This can only make it easier to load the library in unusual ways, not just in NB.

Leaving the policy up to the code using JNI also lets it use richer semantics than ${os.arch} and ${os.name}; for example, it could have different libraries for different Mac OS X releases, or for GNOME vs. KDE, etc.

If we need to enhance the module system to work around poorly designed third-party libraries which cannot be fixed upstream, the attached patch is not the way I would do it. Rather, the NB wrapper module should somehow inform the module system of a mapping from logical name to File, either using a procedural call made before the third-party code which calls loadLibrary is entered, or using some kind of declaratively registered callback.

> org/openide/modules/doc-files/api.html#jni

What "proposal" are you referring to there, exactly? The doc quite explicitly describes what the implementation in fact does:

"The module is entirely responsible for distinguishing between various platforms and operating systems and requesting a library name appropriate to the current one"

> I consider the current state a P2 bug,
> I propose applying it for 6.9.

I do not consider this a bug at all - the system is working as designed and documented, and as it has worked for years - and I certainly don't see why it has to happen for 6.9 a couple of days before code freeze. (Who is using it?) Certainly it cannot happen without an API review.


Y01 - an advantage of modules/lib is that the module loader need not be aware of cluster structure; it simply uses "./lib" relative to the location of the module JAR, wherever that might be. I don't see any reason to change this. Anyway we already use $cluster/lib/ for other purposes (startup JARs). If a native package really cared about putting JARs into /usr/share/lib and SOs into /usr/lib, it can already do so using symlinks.
Comment 3 Petr Nejedly 2010-05-12 15:52:04 UTC
Oops, I quoted wrong URL. I have based my implementation on the following document: http://platform.netbeans.org/articles/installation.html#logical

"If it proves unsufficient following conventions shall be obeyed: There are subdirectories under the lib directory that reflect the architecture (e.g. i386, obtained via System.getProperty ("os.arch")) and possibly also subdirectory identifying the system (e.g. i386/linux, obtained via System.getProperty ("os.name").toLowerCase ()). When the system is looking for a particular library it scans the deepest directories first and then the shallow ones (lib/i386/linux, lib/i386, lib)."

> The recommended solution is to fix those libraries to call loadLibrary with
> distinctive names according to platform. This can only make it easier to load
> the library in unusual ways, not just in NB.
As long as you can fix them. Though I must admit I haven't tried to convince rxtx maintainer to do so, in the light of the above mentioned citation (".. shall ..")

> Rather, the NB wrapper module should somehow inform the
> module system of a mapping from logical name to File, either using a procedural
> call made before the third-party code which calls loadLibrary is entered, or
> using some kind of declaratively registered callback.
Well, rather than doing that, I'd well may end up keeping my current:
    Field f = ClassLoader.class.getDeclaredField("usr_paths");
    f.setAccessible(true);
    ...

> I do not consider this a bug at all - the system is working as designed and
> documented, 
Well, at least according to one doc, not according to another doc.

> and as it has worked for years
There were 3rd party developer problems with this, those 3rd parties were worried that it doesn't behave as specified (in the mentioned article) and they have worked the problem around somehow. And there have been more cases, as the mailing list archive contains a query from one developer and an answer from another that it doesn't work as described and how they have solved this.
I don't have the pointers at hand, but I learned about the problem and the article when I was googling around for a solution for my (with my 3rd party hat on) problem loading rxtx.
Maybe we think it worked because we don't use native library that much?

>  why it has to happen for 6.9 a couple of days before code freeze
Because I have only learned about the problem this late, when I switched to 64bit OS and thought my fix is the preferred solution that nobody has had a chance/need to implement before.

BTW: core.nativeaccess does the dirty work of copying files during startup,
extbrowser module does select its file itself, as you describe.
libs.jna apparently handles the copying of the right native library out of its jar itself (BTW: the same library as core.nativeaccess, maybe different version)

Y02 looks relevant.
Comment 4 Jesse Glick 2010-05-12 16:45:55 UTC
(In reply to comment #3)
>> the system is working as designed and documented
>
> Well, at least according to one doc, not according to another doc.

The module system API document is the specification of what modules may rely upon for JNI loading. The URL you link to is an old and partially implemented proposal for layout of build-time artifacts; it is not an official specification for use by module developers, which is why it is not part of the Javadoc.

> libs.jna apparently handles the copying of the right native library out of its
> jar itself

Yes, JNA makes it automatic.

> BTW: the same library as core.nativeaccess, maybe different version

core.nativeaccess uses libs.jna. It seems to do some copying into the cache dir during first start as an optimization, since JNA permits you to override its default extraction mechanism. (This code should likely reside in libs.jna/src instead, since there are other modules using JNA for unrelated reasons, e.g. keyring.)

>> the NB wrapper module should somehow inform the
>> module system of a mapping from logical name to File
>
> rather than doing that, I'd well may end up keeping my current:
>     Field f = ClassLoader.class.getDeclaredField("usr_paths");

That relies on unspecified details of the Sun JRE, whereas using ClassLoader.findLibrary is reliable and compliant.

> Y02 looks relevant.

Not if the code looks for the lib in the right place to begin with.
Comment 5 Jan Lahoda 2010-05-13 07:12:19 UTC
(In reply to comment #4)
> >> the NB wrapper module should somehow inform the
> >> module system of a mapping from logical name to File
> >
> > rather than doing that, I'd well may end up keeping my current:
> >     Field f = ClassLoader.class.getDeclaredField("usr_paths");
> 
> That relies on unspecified details of the Sun JRE, whereas using

Yes. But I know about only two ways to make the rxtx library work in NB: using the above reflection call or patch the library. Even copying the resource on startup is not a good solution, as the library would need to be copied into ${module-jar}/../lib, which may not be a writable location (copying the lib unconditionally into userdir does not work, AFAIK).

> ClassLoader.findLibrary is reliable and compliant.

Yes. Which is what this report is about, I think.
Comment 6 Jaroslav Tulach 2010-05-13 10:01:21 UTC
Re. "Jesse on Y01" - disadvantage of dealing directly with File(...) is that the system cannot benefit from optimizations made in InstalledFileLocator impl. E.g. there would need to be some special solution for Y02. On the other hand, if InstalledFileLocator is used, Y02 will be addressed almost automatically (as there already is cache of all files under all clusters). I see no problem using InstalledFileLocator from org.netbeans.bootstrap classloaders. Should that be an issue, using an injection bridge is the proper solution.

Re. "The URL you link to is an old and partially" - the document may be implemented partially, as nobody found something is broken yet. The document is not proposal, it is description how the system shall work (the document is shortened version of original proposal). I believe that discovered divergences between reality and the document are bugs (we can of course fix the document). Btw. If moving the document to javadoc of openide.modules is desirable, I can handle it.

"Leaving the policy up to the code using JNI" - this is always an option, not affected by Petr's proposal. However in times where 32 vs. 64 versions of operating systems are common, there shall be standard, declarative way of providing flavors of native libraries. If the installation structure document mentioned in Y01 satisfies the needs, let's make sure we implement it according to already reviewed and approved specification.
Comment 7 Jesse Glick 2010-05-13 13:35:00 UTC
(In reply to comment #5)
> I know about only two ways to make the rxtx library work in NB

You can always load third-party code in a custom ClassLoader that does native linking however you like, BTW. Of course it means you need at least one call to reflection, minimally:

CommunicationWithRxtx bridge = (CommunicationWithRxtx) customLoader.loadClass("...CommunicationWithRxtxImpl").newInstance();

where CommunicationWithRxtx is in the module, and CommunicationWithRxtxImpl and rxtx.jar are loaded from customLoader, e.g. a URLClassLoader overriding findLibrary.

(In reply to comment #6)
> there would need to be some special solution for Y02.

Again, if you tell ClassLoader exactly the name to use, the disk will be accessed only once to load that file, so I am not sure what you are talking about.

> there already is cache of all files under all clusters

Not sure how this relates to anything. You give ClassLoader a path to load, and it loads the file at that path. No cache is needed.

> it is description how the system shall work

Of how NB installations should conventionally be laid out for purposes of native packaging. This is different than a description of what arbitrary modules may expect the module system to do in all circumstances, which is what the Modules API should generally be limited to.

> in times where 32 vs. 64 versions of
> operating systems are common, there shall be standard, declarative way of
> providing flavors of native libraries

I disagree; I see no need to force a very limited declarative syntax onto something which could just as easily (and far more flexibly) be done using a short section of lightweight Java code run immediately before loading the native library. Otherwise we risk wasting time on a complex and still limited system such as section 3.9.1 of the OSGi spec.
Comment 8 Jaroslav Tulach 2010-05-13 14:08:16 UTC
As far as I can see Jesse is finding ways to hack around the problem:

(In reply to comment #7)
> You can always load third-party code in a custom ClassLoader that does native

Hack.

> Again, if you tell ClassLoader exactly the name to use

Relies on hack - can't work for existing libraries when loaded via NetBeans module system ClassLoader.

> > in times where 32 vs. 64 versions of
> > operating systems are common, there shall be standard, declarative way of
> > providing flavors of native libraries
> 
> I disagree; I see no need to force a very limited declarative syntax onto
> something which could just as easily (and far more flexibly) be done using a

Done via a complex hack.

On contrary to Jesse's attitude of not solving anything, just hacking, I am not afraid to solve the issues along we face them. The installation document...

> > it is description how the system shall work
> 
> Of how NB installations should conventionally be laid out 

... describes exactly how the native libraries files shall be organized in any form of distribution including ZIP files. The fact the proposal has not been implemented yet, is just a sign of nobody needing it so far. Now, when the need is clear. I see no reason to wait. It is a bug this feature does not work yet.

> native library. Otherwise we risk wasting time on a complex and still limited
> system such as section 3.9.1 of the OSGi spec.

I am more concern by wasting time discussing reason why not let Petr implement what shall have been implemented for ages. Agile API design is about addressing issues that are painful with long time sustainable APIs. Once/if we find additional requirements, we can enhance the semantics. If Petr is willing to do the work and adhere to comments gathered during API Review, the risks associated with such change are minimal/none.
Comment 9 Jesse Glick 2010-05-13 15:59:55 UTC
(In reply to comment #8)
>> You can always load third-party code in a custom ClassLoader
> 
> Hack.

Way to make things work reliably using the current NB module system without patching a third-party library or relying on Sun JRE impl details. I was not suggesting the above as a long-term solution; I was only replying to the assertion that it is impossible to load such native code reliably using current NB releases, which I believe is not true - it is just awkward.

>> if you tell ClassLoader exactly the name to use
> 
> can't work for existing libraries when loaded via NetBeans
> module system ClassLoader

And why not? The legacy library will ask to load some generic name like "mylib", expecting to get mylib.so from $LD_LIBRARY_PATH of something like .../linux-gtk-x64-nvidia, or however the developer of this library expected things to be installed in a non-modular world. The NB module system (OneModuleClassLoader.findLibrary) will ask the wrapper module (using some SPI to be defined) where to find "mylib", and it will reply with a File path, typically using InstalledFileLocator. (Or it could reply with a String path relative to the module JAR location, as now, permitting the core module system to remain agnostic about installation structure.)

Quite straightforward. The only difference from the current state is that the module is permitted to alter the details of the implementation of findLibrary. It can do something simple, or something complicated; we don't need to decide in advance what possible criteria might be needed to select among similarly-named DLLs.
Comment 10 Petr Nejedly 2010-05-13 20:39:30 UTC
> You can always load third-party code in a custom ClassLoader

You seem to not understand the use case at hand. The library (a native shared object + a nontrivial set of classes) might do much more than to define a few entry points. rxtx for that matter completely encapsulates the native part with a wide java OO API with many types and everything. The fact that the APIs are backed by native implementation is a hidden implementation detail. Except the need to have the right file on the library path.

> OneModuleClassLoader.findLibrary will ask the wrapper module (using some SPI
> to be defined) where to find "mylib", and it will reply with a File path.

Yes, this would work, though is much more complicated than the proposed scheme. It would require an SPI, a way to register it and the module in question would need to implement it. On the other hand, the need for such a flexibility is not there so far, but even if it arises later, it could be implemented as a compatible evolution of the behavior - with the proposed behavior being implemented as the default impl of the SPI.
Comment 11 Petr Nejedly 2010-05-13 21:41:46 UTC
Created attachment 98971 [details]
Updated patch

Updated the patch to address Y02 - uses InstalledFileLocator now, which (inside the running platform) should know what files are present and not touch the disk otherwise.
Comment 12 Jaroslav Tulach 2010-05-14 08:25:54 UTC
> I was only replying to the
> assertion that it is impossible to load such native code reliably using current
> NB releases, which I believe is not true - it is just awkward.

I see. It looked like seriously meant long term solution proposal.

> The NB module system
> (OneModuleClassLoader.findLibrary) will ask the wrapper module (using some SPI
> to be defined) where to find "mylib", and it will reply with a File path,

I see - a complex proposal.

So I guess we have three choices. 1. Do nothing and let solutions remain awkward. 2. Apply Nejedlák's patch and solve our current problems in reasonable way. 3. design something more complex and powerful.

For me #2 seems like good balance: Stretches the itch + is not ugly + can be improved in future. I believe we shall accept #2 and wait for additional itches.

Independent to this is question of timing. The change arrived too late in the release cycle. Should there be any concerns about instability, it would have to wait for next release. 

From my point of view, if the number of disk touches for NetBeans IDE is not increased, I am not worried to accept the change.

And something for Nejedlák:
Y03 - Make sure the new behavior is documented in javadoc, probably at
http://bits.netbeans.org/dev/javadoc/org-openide-modules/org/openide/modules/doc-files/api.html#jni
Comment 13 ivan 2010-05-14 09:01:16 UTC
Petr, can you please write up the layout as a spec/comment so 
there's no mistaken intrpretation of the code semantics?

My concern is over a clash of this new system and some 
existing ad-hoc system. Won't you get the ad-hoc as well as 
this new system competing for who/how the libraries are loaded?
Comment 14 Petr Nejedly 2010-05-14 10:29:53 UTC
> Petr, can you please write up the layout as a spec/comment so 
> there's no mistaken intrpretation of the code semantics?
Sure, that's Y03

> My concern is over a clash of this new system and some 
> existing ad-hoc system. Won't you get the ad-hoc as well
> as this new system competing for who/how the libraries are loaded?
To be safer, I will change the probing order to:
* modules/lib
* modules/lib/arch
* modules/lib/arch/os
and provide a test that would fail otherwise.

Potential ad-hoc soultions would have had to really load the library from modules/lib anyway (not counting my ugly reflective hack, comment #3, which might have had enabled any path).
Comment 15 Petr Nejedly 2010-05-14 12:42:40 UTC
Created attachment 99004 [details]
A candidate for a final patch

I have attached a new patch. It adresses Y03 - adds a documentation and notes an API change.
I have also changed the search order to increase the compatibility of proposed solution.
Comment 16 Jesse Glick 2010-05-14 19:13:38 UTC
(In reply to comment #10)
>> You can always load third-party code in a custom ClassLoader
> 
> The library might do much more than to define a few entry points.

It can have as many entry points as you like. The NB module would still communicate with it through the interface. Anyway this is more or less off topic.

> this would work, though [it] is much more complicated than the proposed scheme.
> It would require an SPI, a way to register it

Could be as simple as a new method in ModuleInstall.

> and the module in question would need to implement it.

Naturally - as a well-written library already would.

Which version is "much more complicated" is a matter of perspective. We are comparing:

1. [current state] $LD_LIBRARY_PATH is $jar/../lib.

2. [mine] As #1, unless ModuleInstall.<whatever> says otherwise.

3. [yours] $LD_LIBRARY_PATH is $jar/../lib plus $jar/../lib/${os.arch} plus $jar/../lib/${os.arch}/${os.name.toLowerCase}, plus any additional combinatorial variants we might add in the future (as the OSGi maintainers felt necessary to do) or refinements to system property normalization, all to work provided that the module is in fact in modules/*.jar and an InstalledFileLocator is present in the platform and functioning according to usual NBM conventions; plus perhaps #2 as well if even these changes do not suffice in some cases.

If I'm outnumbered, so be it - just recording my objections to this approach.
Comment 17 Petr Nejedly 2010-05-16 22:54:16 UTC
So unless there are further objections, and working hg.netbeans.org permitting, I'll apply the last patch Monday morning.
Comment 18 David Simonek 2010-05-18 07:00:25 UTC
Petr what is the status, will you integrate before code freeze? Just curious, thanks.
Comment 19 Petr Nejedly 2010-05-18 08:15:33 UTC
Yes, I have integrated it yesterday to core-main:
http://hg.netbeans.org/core-main/rev/7cd699e820cc
and breathlessly watched the propagation to other repositories:
http://hg.netbeans.org/main/rev/7cd699e820cc
http://hg.netbeans.org/main-silver/rev/7cd699e820cc

Now, I agree with Jesse that ModuleInstall method would be much more powerful,
though such path would need a ModuleInstall class (something usually not present in a library wrapper module) and would be less compatible (adding a method to a public extendable class always* brings a risk of signature collision, especially if somebody tried to solve native library loading problem somehow), thus more risky for 6.9.

*) except if you bring in a new type.

If there is a need for more powerful scheme, we can switch to ModuleInstall way compatibly later, but this way, we at least have standardized method for most common cases.

BTW: I would vote for rewriting JNA module to use this mechanism now that we have such a solution. It would not need to copy the right library version to the userdir anymore.
Comment 20 Jaroslav Tulach 2010-05-18 08:35:37 UTC
Extending ModuleInstall is tempting. But too powerful. I'd rather see declarative approach covering current usecases, than procedural style (method in ModuleInstall) which will get things out of our control completely.

In case you are not scared yet, then (in like with the Jesse's proposal) I want to add 

public File[] getExtraClassPath();

into ModuleInstall too! As soon as Jesse starts to think this is good idea, I will be OK with File ModuleInstall.loadLibrary(String s);

Re. "rewriting JNA module to use this mechanism" - report a bug; propose a patch.
Comment 21 Jesse Glick 2010-05-18 15:15:32 UTC
(In reply to comment #19)
> such path would need a ModuleInstall class (something usually not
> present in a library wrapper module)

So, add one. This is just one possible place to put the API, that I think would seem most natural. You could also define a new interface registered using @ServiceProvider, though the code to look it up should not use Lookup.getDefault but rather

ClassLoader l = thisModule.getClassLoader();
for (Whatever w : ServiceLoader.load(Whatever.class, l)) {
  if (w.getClass().getClassLoader() == l) {
    // call it
  }
}

to avoid picking up a similar service from an upstream module. (If we ever switch to registering ModuleInstall's as "regular" services we would need to use the same check.)

(In reply to comment #20)
> (in line with the Jesse's proposal) I want to add 
> 
> public File[] getExtraClassPath();
> 
> into ModuleInstall too

I would not propose that and I don't see how it is line with determining the native library path dynamically. Bundled JARs do not need to be swapped according to platform, due to Java's nature, whereas bundled JNI libs do. Modules which rely on external JAR files (not part of the NBM fileset) really need to load those JARs in a separate URLClassLoader (and be prepared for missing files or various sorts of linkage errors).
Comment 22 Quality Engineering 2010-05-19 06:17:16 UTC
Integrated into 'main-golden', will be available in build *201005182201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/
User: 
Log:
Comment 23 Jaroslav Tulach 2010-05-19 08:29:13 UTC
> (In reply to comment #20)
> > (in line with the Jesse's proposal) I want to add 
> > 
> > public File[] getExtraClassPath();
> > 
> > into ModuleInstall too
> 
> I would not propose that and I don't see how it is line with determining the
> native library path dynamically. Bundled JARs do not need to be swapped
> according to platform, due to Java's nature, whereas bundled JNI libs do.
> Modules which rely on external JAR files (not part of the NBM fileset) really
> need to load those JARs in a separate URLClassLoader (and be prepared for
> missing files or various sorts of linkage errors).

Exactly as expected! What could a person rejecting any changes to config/Modules/*.xml to make loading of external JARs easier say? Everyone on the planet just needs to learn what URLClassLoader is, am I not right ;-?

I have to mention that I disagree. Even modules that "rely on external JAR files" could just locate them without playing dirty tricks with URLClassLoader. 

But my main target is this kind of "inconsistent mind state". Should computing library paths programatically be OK (and I am against that), then it shall be OK to compute classpath too.
Comment 24 Jesse Glick 2012-05-02 19:41:27 UTC
So the only use of this API in the current NB distribution, libs.jna, was using it incorrectly; fixing in core-main #2ed9bef1c084.

The bigger question is what, exactly, ${os.name} and ${os.arch} are going to be. "Mac OS X" or "Darwin"? "amd64" or "x86_64" or "ia64"? [1] asks the question, but the answer is unclear; it indirectly links to [2] but that is laughably outdated. Our org.openide.util.Utilities determines OS but only by String.startsWith. com.sun.jna.Platform determines OS, but again using String.startsWith on os.name, and for arch using both os.name and (like our org.netbeans.modules.extbrowser.NbDdeBrowserImpl does) sun.arch.data.model and more.

Eclipse has its own specialized system that takes os.name and os.arch into account, but normalizes them in various ways, and also adds GUI toolkit (e.g. "gtk") as a third coordinate.

[1] http://stackoverflow.com/questions/1803075/crowdsourcing-a-complete-list-of-common-java-system-properties-and-known-values
[2] http://lopica.sourceforge.net/os.html
Comment 25 Quality Engineering 2012-05-04 09:54:28 UTC
Integrated into 'main-golden', will be available in build *201205040400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/2ed9bef1c084
User: Jesse Glick <jglick@netbeans.org>
Log: #186000 spec & impl confusingly lowercases ${os.name} before constructing a path.