Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for finding libraries with multi component versions on Linux #76

Merged
merged 1 commit into from
Sep 21, 2016

Conversation

pepijnve
Copy link
Contributor

@pepijnve pepijnve commented Sep 6, 2016

No description provided.

@@ -448,22 +447,14 @@ public String getName() {
/**
* A {@link Platform} subclass representing the Linux operating system.
*/
private static final class Linux extends Supported {
static final class Linux extends Supported {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package private is for testing I guess ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix minor nits, and we'll merge.

import java.util.Arrays;
import java.util.LinkedList;
import java.util.List;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer that imports be explicit until they get too unwieldy (20 imports).

return bestMatch != null ? bestMatch : mapLibraryName(libName);
}

private static int compareVersions(int[] version1, int[] version2)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our convention is brace-inline, not brace-following.

import java.util.Collections;

public class PlatformTest
{
Copy link
Member

@headius headius Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our convention is brace-inline, not brace-following. The rest of the file is brace-following also.

@headius headius added this to the 2.0.10 milestone Sep 21, 2016
@pepijnve
Copy link
Contributor Author

Fixed the brace style and import issues

@headius
Copy link
Member

headius commented Sep 21, 2016

Thank you!

@headius headius merged commit 4900cc6 into jnr:master Sep 21, 2016
version[i - 1] = Integer.parseInt(parts[i]);
}
}
matches.put(file.getAbsolutePath(), version);
Copy link
Contributor

@nirvdrum nirvdrum Nov 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loses the ordering of the library paths and is proving to be problematic for me. I have both /lib64/librt.so.1 and /lib/librt.so.1, with /lib64 before /lib. I'm going to look at stripping out the /lib part, but it's coming from the "java.library.path" system property so it's likely to be there for others, too. In any event, since the HashMap here loses the ordering, /lib/librt.so1 actually comes before /lib64/librt.so.1 during the best match scan. Since they have the same version number, there's no way for the /lib64 version to win and I wind up with an UnsatisfiedLinkError: /lib/librt.so.1: wrong ELF class: ELFCLASS32.

Copy link
Contributor Author

@pepijnve pepijnve Nov 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple fix is to use a LinkedHashMap instead of a HashMap for the matches Map. That way insertion order will be retained and the original behaviour should be restored. The only caveat is that, continuing on your example, if lib contains a more precies version than lib64 that the lib version will get picked.
Ideally the library loading code should filter out libraries that are not using the correct ABI I guess. That's new functionality that the old code did not do either though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I'd be happy just getting the sort order back. It appears the java.library.path changes were made with JDK 7 to include both 32-bit and 64-bit paths. While I'm not convinced that was a worthwhile change, it's something we'll have to cope with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 9143ba2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants