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.

Sign up for GitHub

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

Jump to bottom

Extract dylib for Mac, Remove Zip Requirement #65

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Extract dylib for Mac, Remove Zip Requirement #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions BWAPI4J/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import org.gradle.api.tasks.testing.logging.TestExceptionFormat
import org.gradle.api.tasks.testing.logging.TestLogEvent

import java.nio.charset.StandardCharsets
import java.nio.file.Files
import java.nio.file.Paths

group 'org.openbw.bwapi4j'
version '0.8.2b'
Expand Down Expand Up @@ -36,7 +34,6 @@ dependencies {
compile 'org.apache.logging.log4j:log4j-core:2.11.0'
compile 'org.apache.commons:commons-lang3:3.7'
compile 'com.github.xaguzman:pathfinding:0.2.6'
compile 'net.lingala.zip4j:zip4j:1.3.2'

testCompile 'junit:junit:4.12'
testCompile 'org.apache.commons:commons-compress:1.16.1'
Expand All @@ -45,10 +42,10 @@ dependencies {
shadowJar {
relocate 'org.apache.commons', 'org.openbw.bwapi4j.org.apache.commons'
relocate 'org.xguzm', 'org.openbw.bwapi4j.org.xguzm'
relocate 'net.lingala.zip4j', 'org.openbw.bwapi4j.net.lingala.zip4j'
classifier = null
from ('../BWAPI4JBridge/Release') {
include '*.dll'
include '*.dylib'
include '*.so'
}
}
Expand Down Expand Up @@ -124,8 +121,7 @@ task generateJniHeaders(type:Exec) {
"org.openbw.bwapi4j.InteractionHandler",
"org.openbw.bwapi4j.MapDrawer",
"org.openbw.bwapi4j.DamageEvaluator",
"org.openbw.bwapi4j.unit.UnitImpl",
"net.lingala.zip4j.core.ZipFile" //javah complains if this is not specified
"org.openbw.bwapi4j.unit.UnitImpl"

dependsOn classes
}
Expand Down
65 changes: 25 additions & 40 deletions BWAPI4J/src/main/java/org/openbw/bwapi4j/BW.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,27 @@
package org.openbw.bwapi4j;

import java.awt.image.ColorModel;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.charset.UnsupportedCharsetException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.*;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import net.lingala.zip4j.core.ZipFile;
import net.lingala.zip4j.exception.ZipException;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.openbw.bwapi4j.type.UnitType;
Expand Down Expand Up @@ -223,14 +223,11 @@ private void extractBridgeDependencies(final BridgeType bridgeType) {
&& jarFile.toString().endsWith(".jar")) {
logger.debug("Extracting dependencies from: " + jarFile.toString());

final ZipFile jar = new ZipFile(jarFile.toFile());

extractFileIfNotExists(
jar, resolvePlatformLibraryFilename(bridgeType.getLibraryName()), cwd.toString());

resolvePlatformLibraryFilename(bridgeType.getLibraryName()), cwd.toString());
for (final String externalLibrary : getExternalLibraryNames()) {
extractFileIfNotExists(
jar, resolvePlatformLibraryFilename(externalLibrary), cwd.toString());
resolvePlatformLibraryFilename(externalLibrary), cwd.toString());
}
}
} catch (final Exception e) {
Expand All @@ -240,14 +237,16 @@ private void extractBridgeDependencies(final BridgeType bridgeType) {
}
}

private static void extractFileIfNotExists(
final ZipFile zipFile, final String sourceFilename, final String targetDirectory)
throws ZipException {
final Path targetFile = Paths.get(targetDirectory, sourceFilename);
if (!Files.isRegularFile(targetFile)
&& !Files.isDirectory(targetFile)
&& !Files.exists(targetFile)) {
zipFile.extractFile(sourceFilename, targetDirectory);
private static void extractFileIfNotExists(final String sourceFilename, final String targetDirectory) {
final File library = new File(targetDirectory, sourceFilename);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Path is more modern and robust than File. https://docs.oracle.com/javase/tutorial/essential/io/legacy.html . Please provide justification for using File over Path in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of the changes but will migrate to using Path if I am able to convert to using getResourceAsStream.

if (!library.exists()) {
InputStream is = BW.class.getResourceAsStream(File.separator + sourceFilename);
try {
Files.copy(is, library.toPath(), StandardCopyOption.REPLACE_EXISTING);
} catch (IOException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the justification for catching an IOException and not rethrowing it? This makes debugging more difficult as the library will fail to load and we won't know exactly why. Here, we would be able to see it's not extracting properly. It should be thrown in some way. It could be wrapped in an runtime exception. E.g. throw new RuntimeException(e);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error can be re-thrown on cleanup, but I think I'll remove this since I'd have to test external jar first.

//noinspection ResultOfMethodCallIgnored
library.delete();
}
}
}

Expand Down Expand Up @@ -331,11 +330,6 @@ private List<String> getExternalLibraryNames() {

return libNames;
}

private static String getLibraryPathDelimiter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are now at the mercy of the JDK/JRE deciding what the proper delimiter is. But, I am willing to try File.pathSeparator out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have there been instances where the JVM is detecting the wrong system? If so I would just consolidate the methods since the logic to determine the delimiter is duplicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No instances that I know of nor for which I have any evidence. As I said, we can try it out no big deal and it's probably fine. I just like having more control over simple things rather than relying on a library (even the JDK) to probe the system for something simple.

return isWindowsPlatform() ? ";" : ":";
}

private static boolean isWindowsPlatform() {
return System.getProperty("os.name").contains("Windows");
}
Expand Down Expand Up @@ -394,7 +388,7 @@ private void addLibraryPath(final String path) {

logger.info("Adding library path: {}", path);

final String libraryPathDelimiter = getLibraryPathDelimiter();
final String libraryPathDelimiter = File.pathSeparator;
final String newLibraryPath =
currentLibraryPath
+ (!currentLibraryPath.endsWith(libraryPathDelimiter) ? libraryPathDelimiter : "")
Expand Down Expand Up @@ -433,27 +427,18 @@ private static boolean systemPropertyEquals(
}

private static String resolvePlatformLibraryFilename(String libraryName) {
if (isWindowsPlatform()) {
if (!libraryName.toLowerCase(Locale.US).endsWith(".dll")) {
libraryName += ".dll";
}
} else {
if (!libraryName.startsWith("lib")) {
libraryName = "lib" + libraryName;
}

if (!libraryName.toLowerCase(Locale.US).endsWith(".so")) {
libraryName += ".so";
}
switch (OSType.computeType()) {
case WINDOWS:
return libraryName + ".dll";
case MAC:
return "lib" + libraryName + ".dylib";
default:
return "lib" + libraryName + ".so";
}

return libraryName;
}

private static boolean isPathFoundInPathVariable(final String pathVariable, final String path) {
final String delim = isWindowsPlatform() ? ";" : ":";

final String[] paths = pathVariable.split(delim);
final String[] paths = pathVariable.split(File.pathSeparator);

for (final String directory : paths) {
final Path targetDirectory;
Expand Down