[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349570247 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NativeLibFinder.java ## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; + +public interface NativeLibFinder { Review comment: I think all 3 solutions (member, abstract superclass, trait interface) have their pros and cons. In my opinion the trait interface is the best usually, but it's hard to tell. In this case probably the abstract superclass wins out. **Member (composition)** Pros - Can be reused - Further abstraction and extension not limited - Proper isolation - State code can be written once Cons - More glue code (plus one field, instantiation, data transfer) - Need to make sure states are properly set up in order. (Must not reference the member before it's set up properly. Can be mitigated if immutable/stateless.) **Abstract superclass (Regular inheritance)** Pros - Least amount of glue code - Proper isolation - State setup is usually not an issue - State code can be written once - State can be accessed anywhere Cons - Reusability it very restricted (probably not an issue in this case) - Restricts further abstraction and extension **Trait interface (Behavioural inheritance)** Pros - Minimal/Least amount of glue code - Can be reused - Further abstraction and extension not limited - No issues with state Cons - Isolation is very poor (everything is public) - Can't have state (although can be mitigated by defining and referencing accessors) All in all, abstract superclasses have their well-known issues. Trait interfaces are basically composition 2.0 except Java doesn't really implement this concept very well. The thing is, no matter the use-case, these issues with the trait interfaces will be there. So we basically either accept them with their flaws as a viable approach or never use them at all. I think the cons are not that bad. However in this case adding another classloader superclass fits in the existing picture well so I'll go with that. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349586981 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NarClassLoader.java ## @@ -116,10 +122,13 @@ * Maven NAR plugin will fail to build the NAR. * Review comment: I'll update the docs. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349557277 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NativeLibFinder.java ## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; + +public interface NativeLibFinder { +Logger LOGGER = LoggerFactory.getLogger(NativeLibFinder.class); + +String OS = System.getProperty("os.name").toLowerCase(); + +List getNativeLibDirs(); + +Map getNativeLibNameToPath(); + +String getTmpLibFilePrefix(); + +default Set getUsrLibDirs() { +Set usrLibDirs = Arrays.stream(getJavaLibraryPath().split(File.pathSeparator)) +.map(String::trim) +.filter(pathAsString -> !pathAsString.isEmpty()) +.map(File::new) +.map(toDir()) +.filter(Objects::nonNull) +.collect(Collectors.toSet()); + +return usrLibDirs; +} + +default String getJavaLibraryPath() { +return System.getProperty("java.library.path", ""); +} + +default Function toDir() { +return file -> { +if (file.isFile()) { +return file.getParentFile(); +} else if (file.isDirectory()) { +return file; +} else { +return null; +} +}; +} + +default String findLibrary(String libname) { +Path libLocation = getNativeLibNameToPath().compute( +libname, +(__, currentLocation) -> +Optional.ofNullable(currentLocation) +.filter(Objects::nonNull) +.filter(location -> location.toFile().exists()) +.orElseGet( +() -> getNativeLibDirs().stream() +.map(nativeLibDir -> findLibrary(libname, nativeLibDir)) +.filter(Objects::nonNull) +.findFirst() +.map(libraryOriginalPathString -> createTempCopy(libname, libraryOriginalPathString)) +.orElse(null) +) +); + +String libLocationString = Optional.ofNullable(libLocation) +.map(Path::toFile) +.map(File::getAbsolutePath) +.orElse(null); + +return libLocationString; +} + +default Path createTempCopy(String libname, String libraryOriginalPathString) { +Path tempFile; + +try { +tempFile = Files.createTempFile(getTmpLibFilePrefix() + "_", "_" + libname); +Files.copy(Paths.get(libraryOriginalPathString), tempFile, REPLACE_EXISTING); +} catch (Exception e) { +LOGGER.error("Couldn't create temporary copy of the library '" + libname + "' found at '" + libraryOriginalPathString + "'", e); + +tempFile = null; +} + +return tempFile; +} + +default String findLibrary(String libname, File nativeLibDir) { +final File dllFile = new File(nativeLibDir, libname + ".dll"); +final File dylibFile = new File(nativeLibDir, libname + ".dylib"); +final File libdylibFile = new File(nativeLibDir, "lib" + libname + ".dylib"); +final File libsoFile = new File(nativeLibDir, "lib" + libname + ".so"); +final File soFile = new File(native
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349586673 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/test/java/org/apache/nifi/nar/NativeLibFinderTest.java ## @@ -0,0 +1,566 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; + +public class NativeLibFinderTest { +public static final String NATIVE_LIB_NAME = "native_lib"; + +@Mock +private NativeLibFinder testSubjectHelper; + +private String javaLibraryPath; + +private Path tempDirectory; +private List nativeLibDirs; +private Map nativeLibNameToPath; + +private boolean isOsWindows; +private boolean isOsMaxOsx; +private boolean isOsLinux; + +@Before +public void setUp() throws Exception { +initMocks(this); +tempDirectory = Files.createTempDirectory(this.getClass().getSimpleName()); +} + +@After +public void tearDown() throws Exception { +tempDirectory.toFile().deleteOnExit(); + +Files.walk(tempDirectory) +.sorted(Comparator.reverseOrder()) +.map(Path::toFile) +.forEach(File::delete); + +} + +@Test +public void testFindLibraryShouldReturnNullOnWindowsWhenNoDLLAvailable() throws Exception { +// GIVEN +isOsWindows = true; + +createTempFile("so"); +createTempFile("lib", "so"); +createTempFile("dylib"); +createTempFile("lib", "dylib"); + +String expected = null; + +// WHEN +// THEN +testFindLibrary(expected); +} + +@Test +public void testFindLibraryShouldReturnDLLOnWindows() throws Exception { +// GIVEN +isOsWindows = true; + +Path expectedNativeLib = createTempFile("dll"); +createTempFile("so"); +createTempFile("lib", "so"); +createTempFile("dylib"); +createTempFile("lib", "dylib"); + +String expected = expectedNativeLib.toFile().getAbsolutePath(); + +// WHEN +// THEN +testFindLibrary(expected); +} + +@Test +public void testFindLibraryShouldReturnNullOnMacWhenNoDylibOrSoAvailable() throws Exception { +// GIVEN +isOsMaxOsx = true; + +createTempFile("dll"); + +String expected = null; + +// WHEN +// THEN +testFindLibrary(expected); +} + +@Test +public void testFindLibraryShouldReturnDylibOnMac() throws Exception { +// GIVEN +isOsMaxOsx = true; + +createTempFile("dll"); +createTempFile("so"); +createTempFile("lib", "so"); +Path expectedNativeLib = createTempFile("dylib"); +createTempFile("lib", "dylib"); + +String expected = expectedNativeLib.toFile().getAbsolutePath(); + +// WHEN +// THEN +testFindLibrary(expected); +} + +@Test +public void testFindLibraryShouldReturnLibDylibOnMac() throws Exception { +// GIVEN +isOsMaxOsx = true; + +createTempFile("dll"); +createTempFile("so"); +createTempFile("lib", "so"); +Path expectedNativeLib = createTempFile("lib", "dylib"); + +String expected = expectedNativeLib.toFile().getAbsolutePath(); + +// WHEN +// THEN +testFindLibrary(expected); +
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349537862 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NativeLibFinder.java ## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; + +public interface NativeLibFinder { +Logger LOGGER = LoggerFactory.getLogger(NativeLibFinder.class); + +String OS = System.getProperty("os.name").toLowerCase(); + +List getNativeLibDirs(); + +Map getNativeLibNameToPath(); + +String getTmpLibFilePrefix(); + +default Set getUsrLibDirs() { +Set usrLibDirs = Arrays.stream(getJavaLibraryPath().split(File.pathSeparator)) +.map(String::trim) +.filter(pathAsString -> !pathAsString.isEmpty()) +.map(File::new) +.map(toDir()) +.filter(Objects::nonNull) +.collect(Collectors.toSet()); + +return usrLibDirs; +} + +default String getJavaLibraryPath() { +return System.getProperty("java.library.path", ""); +} + +default Function toDir() { +return file -> { +if (file.isFile()) { +return file.getParentFile(); +} else if (file.isDirectory()) { +return file; +} else { +return null; +} +}; +} + +default String findLibrary(String libname) { +Path libLocation = getNativeLibNameToPath().compute( Review comment: In fact I have started with the imperative approach and switched to this style for the very reason that I think it's more understandable and more robust. I guess I got used to this style by now. And although I admit this can be abused (and I'm probably guilty of that sometimes), in my opinion, even considering code quality - readability and robustness-, the functional approach has objective advantage over the imperative one: Functional style forces the divide and conquer strategy on the programmer (which is very helpful when dealing with more complex problems). It's about breaking down a process into sequential phases where each phase is separated from the rest (not unlike how NiFi works). One can understand the logic step by step which helps readability and since the steps are completely isolated from each other and the general flow is more declarative the overall code is less prone to bugs. Also it's easier to write a cleaner code imo as the declarative nature imposes boundaries that guides the flow. As I said I started with an imperative approach which had 3 private methods and about twice as much code as this. _Now_ I _can_ write it almost the same way as the current functional style, but only because _now_ I see the near-optimal solution. Nevertheless I'll commit a change with a more traditional code style and leave the current one as an unused method for all to be able to compare. After taking a look at both we can have a final decision which to use. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349527062 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NarClassLoader.java ## @@ -205,26 +216,41 @@ private void updateClasspath(File root) throws IOException { } @Override -protected String findLibrary(final String libname) { +public String findLibrary(String libname) { Review comment: The problem is that the abstract `ClassLoader` has an implementation returning null that takes precedent over the interface's default implementation. But I guess this ties back to the bigger question of wether is it even a good idea to use an interface as a trait instead of a member or an abstract superclass. (I give my opinion on that on that specific comment.) (Actually `ClassLoader` defines `findLibrary` as _protected_, which left alone wouldn't even compile because it would mean it would try to override the visibility of the _public_ definition of the interface. This actually points out a weakness of the trait interface approach. Had the ClassLoader defined the `findLibrary` as public, it would have been easy to simply leave out this call to the interface - as it would have compiled but wouldn't have worked properly as it would have returned null.) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349575722 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NativeLibFinder.java ## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; + +public interface NativeLibFinder { +Logger LOGGER = LoggerFactory.getLogger(NativeLibFinder.class); + +String OS = System.getProperty("os.name").toLowerCase(); + +List getNativeLibDirs(); + +Map getNativeLibNameToPath(); + +String getTmpLibFilePrefix(); + +default Set getUsrLibDirs() { Review comment: I'm not sure how can it stay testable without increasing the complexity an uncomfortable amount but I'll try to come with something. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved
tpalfy commented on a change in pull request #3894: NIFI-6884 - Native library loading fixed/improved URL: https://github.com/apache/nifi/pull/3894#discussion_r349538192 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NativeLibFinder.java ## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.nar; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; + +public interface NativeLibFinder { +Logger LOGGER = LoggerFactory.getLogger(NativeLibFinder.class); + +String OS = System.getProperty("os.name").toLowerCase(); + +List getNativeLibDirs(); + +Map getNativeLibNameToPath(); + +String getTmpLibFilePrefix(); + +default Set getUsrLibDirs() { +Set usrLibDirs = Arrays.stream(getJavaLibraryPath().split(File.pathSeparator)) +.map(String::trim) +.filter(pathAsString -> !pathAsString.isEmpty()) +.map(File::new) +.map(toDir()) +.filter(Objects::nonNull) +.collect(Collectors.toSet()); + +return usrLibDirs; +} + +default String getJavaLibraryPath() { +return System.getProperty("java.library.path", ""); +} + +default Function toDir() { Review comment: Indeed, good catch. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services