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

Build standalone modules (jars) to be uploaded to remote repositories #83

Merged
merged 40 commits into from Jul 13, 2018

Conversation

Projects
None yet
6 participants
@johanvos
Collaborator

johanvos commented May 23, 2018

In this PR, we deal with the work to be done in order to upload artifacts to maven central and similar repositories.
While jmods already contain the native libraries, they are not in the runtime picture. The current plan is to create jars with native code, and upload those.
The jars-with-native-code should not overwrite the jars used in the standalone SDK (without native libs)

johanvos added some commits May 17, 2018

include native libs in the jar.
Note: this is also including the native libs in the jars for the SDK, which we do not want.
We need to create a separate build directory for jars that are expected to be uploaded to
central repositories and used as standalone, self-containing components.

@eugener eugener added the enhancement label May 27, 2018

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet May 30, 2018

BTW, I've already given this much thought as part of the JavaCPP Presets. It probably doesn't fit JavaFX like a glove, since JavaFX uses Gradle instead of Maven for one thing, but I'm sure a lot of ideas could be useful: https://github.com/bytedeco/javacpp-presets

I would also be very interested in a gradle-javacpp plugin, and if we can consolidate all the work necessary for JavaFX inside such a plug-in, it would automatically become available for Deeplearning4j and the projects that are part of the presets (FFmpeg, OpenCV, TensorFlow, etc), including for platforms like iOS!

saudet commented May 30, 2018

BTW, I've already given this much thought as part of the JavaCPP Presets. It probably doesn't fit JavaFX like a glove, since JavaFX uses Gradle instead of Maven for one thing, but I'm sure a lot of ideas could be useful: https://github.com/bytedeco/javacpp-presets

I would also be very interested in a gradle-javacpp plugin, and if we can consolidate all the work necessary for JavaFX inside such a plug-in, it would automatically become available for Deeplearning4j and the projects that are part of the presets (FFmpeg, OpenCV, TensorFlow, etc), including for platforms like iOS!

johanvos added some commits Jun 3, 2018

use StackWalker.getCallerClass() to find who is calling the NativeLib…
…Loader.

and use that class to load the bytes containing the library-as-a-resource
@kevinrushforth

I added a few comments, but I think nailing down the overall approach first is more important.

The easiest approach might be to have one platform-specific .jar file per module per platform, and a platform-independent .pom file per module that depends on the platform-specific .jar file for that platform.

In addition to not having to worry about any platform-specific class files (which there are in at least the javafx.graphics module), I think it will simplify the build at the expense of hosting 3x the number of jar files for those modules without native code.

As an optimization, we could have a single .jar file for the modules without native code. We would need to ensure that the class files and resources in those jar files are identical (which I think they are, but it would need to be verified). That could be done later I think.

What are your thoughts on this?

Show outdated Hide outdated build.gradle Outdated
Show outdated Hide outdated build.gradle Outdated
Show outdated Hide outdated build.gradle Outdated
Show outdated Hide outdated ...s/javafx.graphics/src/main/java/com/sun/glass/utils/NativeLibLoader.java Outdated
public static synchronized void loadLibrary(String libname) {
if (!loaded.contains(libname)) {
loadLibraryInternal(libname);
StackWalker walker = AccessController.doPrivileged((PrivilegedAction<StackWalker>) () ->
StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE));

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 6, 2018

Contributor

For this method youdon't need AccessController.doPrivileged since the caller needs to assert privileged before calling. It isn't harmful, though.

@kevinrushforth

kevinrushforth Jun 6, 2018

Contributor

For this method youdon't need AccessController.doPrivileged since the caller needs to assert privileged before calling. It isn't harmful, though.

Show outdated Hide outdated ...s/javafx.graphics/src/main/java/com/sun/glass/utils/NativeLibLoader.java Outdated
@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 6, 2018

The easiest approach might be to have one platform-specific .jar file per module per platform, and a platform-independent .pom file per module that depends on the platform-specific .jar file for that platform.

@kevinrushforth Yes, that's exactly how JavaCPP does it. Could you let me know about the reasons why using existing tools such as JavaCPP, among others, isn't being considered?

saudet commented Jun 6, 2018

The easiest approach might be to have one platform-specific .jar file per module per platform, and a platform-independent .pom file per module that depends on the platform-specific .jar file for that platform.

@kevinrushforth Yes, that's exactly how JavaCPP does it. Could you let me know about the reasons why using existing tools such as JavaCPP, among others, isn't being considered?

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 6, 2018

Contributor

One reason is that we have policies on using third-party code. We do that when there is no viable alternative, but otherwise try to avoid additional third-party dependencies.

On the technical front, the bulk of the changes will be in the custom build.gradle logic anyway, which is specific to FX. Finally, we already have a NativeLibLoader method that is used in several places in three different modules. Modifying that one utility method is straight-forward.

Johan can chime in if he has additional insights.

Contributor

kevinrushforth commented Jun 6, 2018

One reason is that we have policies on using third-party code. We do that when there is no viable alternative, but otherwise try to avoid additional third-party dependencies.

On the technical front, the bulk of the changes will be in the custom build.gradle logic anyway, which is specific to FX. Finally, we already have a NativeLibLoader method that is used in several places in three different modules. Modifying that one utility method is straight-forward.

Johan can chime in if he has additional insights.

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 7, 2018

Well, I get that it's not a lot of code to maintain, but it is quite tricky and you will encounter a lot of issues...

BTW, what about including this in the JDK? Why hasn't this been done before?

saudet commented Jun 7, 2018

Well, I get that it's not a lot of code to maintain, but it is quite tricky and you will encounter a lot of issues...

BTW, what about including this in the JDK? Why hasn't this been done before?

@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 7, 2018

Collaborator

I really like JavaCPP, but I see this as a slightlyt different usecase. I use JavaCPP to access existing native libraries via a X-platform Java interface.
In this case, we talk about native libraries we created ourselves, that only contain a very limited number of API's whose signature might even be platform-dependent. The amount of jni code is very limited here, and that's not a problem. Hence, generating the Java code is not needed here.

The concept of the Loader is very relevant here, so we could use it or similar.

Bringing this in the OpenJDK project sounds very good to me, and there are procedures for doing so (e.g. creating a JEP).

Collaborator

johanvos commented Jun 7, 2018

I really like JavaCPP, but I see this as a slightlyt different usecase. I use JavaCPP to access existing native libraries via a X-platform Java interface.
In this case, we talk about native libraries we created ourselves, that only contain a very limited number of API's whose signature might even be platform-dependent. The amount of jni code is very limited here, and that's not a problem. Hence, generating the Java code is not needed here.

The concept of the Loader is very relevant here, so we could use it or similar.

Bringing this in the OpenJDK project sounds very good to me, and there are procedures for doing so (e.g. creating a JEP).

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 7, 2018

Yeah, I know about the JEP, but until I understand why this isn't something that people want, I don't feel inclined to spend my time writing that kind of thing.

saudet commented Jun 7, 2018

Yeah, I know about the JEP, but until I understand why this isn't something that people want, I don't feel inclined to spend my time writing that kind of thing.

johanvos added some commits Jun 8, 2018

build publication artifacts in separate directory (don't override SDK…
… jar)

The build now creates 1 jar per module per compileTarget
Each jar contains all native libs for that jar
The name of the jar includes the compileTarget as classifier
The module name does not include a classifier
@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 8, 2018

Collaborator

The latest changes will build a jar containing classes + libs for each module, for each compileTarget.
The name of the jarfile contains the compileTarget as classifier (e.g. javafx.graphics-linux.jar). The pom files contain classifiers in their dependencies as well (e.g. javafx.controls-linux.jar depend on javafx.graphics-linux.jar). This allows maven to resolve transitive dependencies.

Each jar has a module-info where the name of the module does not contain a compileTarget classifier.

I tested this in 2 ways:

  1. with a pom.xml, where I have a single dependency on openjfx:javafx.controls-linux:11.00-SNAPSHOT
  2. with command-line invocation, with --module-path pointing to the location of the jar files and --add-modules javafx.controls

Both approaches work.

Collaborator

johanvos commented Jun 8, 2018

The latest changes will build a jar containing classes + libs for each module, for each compileTarget.
The name of the jarfile contains the compileTarget as classifier (e.g. javafx.graphics-linux.jar). The pom files contain classifiers in their dependencies as well (e.g. javafx.controls-linux.jar depend on javafx.graphics-linux.jar). This allows maven to resolve transitive dependencies.

Each jar has a module-info where the name of the module does not contain a compileTarget classifier.

I tested this in 2 ways:

  1. with a pom.xml, where I have a single dependency on openjfx:javafx.controls-linux:11.00-SNAPSHOT
  2. with command-line invocation, with --module-path pointing to the location of the jar files and --add-modules javafx.controls

Both approaches work.

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 8, 2018

Contributor

I confirm that the general approach works. A couple general comments:

  1. Media does not work. The libgstreamer-lite.so, libfxplugin.so and the libavplugin-*.so files are not loaded by Java code, but by native code that expects to find them in the same directory as libjfxmedia.so was loaded from. If I manually unzip the files into /tmp then it works. We might need a new method to NativeLibLoader (e.g., "cacheLibaray" or similar) that the Media module can call on its list of libraries that are not loaded via Java.

  2. In order for more than one app to run concurrently, we will either need some form of caching with locking mechanism so they can share the downloaded .jar files or else we will need a unique directory for each invocation. If we use a caching approach, then the directory will need to be versioned (or you will need checksums or similar). Also we will likely need a per-user directory rather than /tmp/ or we could run into filesystem permission problems. Given the complexity, it might be better to go with a unique directory name for each invocation. You will need to think about cleaning it up.

Contributor

kevinrushforth commented Jun 8, 2018

I confirm that the general approach works. A couple general comments:

  1. Media does not work. The libgstreamer-lite.so, libfxplugin.so and the libavplugin-*.so files are not loaded by Java code, but by native code that expects to find them in the same directory as libjfxmedia.so was loaded from. If I manually unzip the files into /tmp then it works. We might need a new method to NativeLibLoader (e.g., "cacheLibaray" or similar) that the Media module can call on its list of libraries that are not loaded via Java.

  2. In order for more than one app to run concurrently, we will either need some form of caching with locking mechanism so they can share the downloaded .jar files or else we will need a unique directory for each invocation. If we use a caching approach, then the directory will need to be versioned (or you will need checksums or similar). Also we will likely need a per-user directory rather than /tmp/ or we could run into filesystem permission problems. Given the complexity, it might be better to go with a unique directory name for each invocation. You will need to think about cleaning it up.

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 9, 2018

Contributor

Also for debugging, it would be handy if a "loaded from ...." message were printed in the maven case when the "javafx.verbose" property is set.

Contributor

kevinrushforth commented Jun 9, 2018

Also for debugging, it would be handy if a "loaded from ...." message were printed in the maven case when the "javafx.verbose" property is set.

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 9, 2018

saudet commented Jun 9, 2018

@kevinrushforth kevinrushforth added the WIP label Jun 9, 2018

@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 12, 2018

Collaborator

Samuel has a good point there, the JavaCPP Loader already deals with a list of preloaded libraries that need to be loaded. It would be nice to have something generic in Java, for now I'll add it to the NativeLibLoader.

Collaborator

johanvos commented Jun 12, 2018

Samuel has a good point there, the JavaCPP Loader already deals with a list of preloaded libraries that need to be loaded. It would be nice to have something generic in Java, for now I'll add it to the NativeLibLoader.

johanvos added some commits Jun 12, 2018

cache native libaries in {user, version}-specific directory
allow to preload dependent native libraries
@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 12, 2018

Collaborator

Libraries are now stored in a cache in the user directory and are version dependent. TODO: don't replace if checksum is the same.
I added a loadLibary method that takes a List containing the required dependencies for a particular native library. This is platform-dependent.

Collaborator

johanvos commented Jun 12, 2018

Libraries are now stored in a cache in the user directory and are version dependent. TODO: don't replace if checksum is the same.
I added a loadLibary method that takes a List containing the required dependencies for a particular native library. This is platform-dependent.

@kevinrushforth

This looks quite close to being ready. Have you tried on all three desktop platforms? I only had time to run on Linux today, but will check Mac and Windows tomorrow.

Show outdated Hide outdated ...s/javafx.graphics/src/main/java/com/sun/glass/utils/NativeLibLoader.java Outdated
Show outdated Hide outdated build.gradle Outdated
if (HostUtils.isWindows() || HostUtils.isMacOSX()) {
NativeLibLoader.loadLibrary("glib-lite");
}
if (!HostUtils.isLinux() && !HostUtils.isIOS()) {
NativeLibLoader.loadLibrary("gstreamer-lite");
} else {
dependencies.add("gstreamer-lite");

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 13, 2018

Contributor

For Linux, the following are also needed as dependencies:

                    if (HostUtils.isLinux()) {
                        dependencies.add("fxplugins");
                        dependencies.add("avplugin-54");
                        dependencies.add("avplugin-56");
                        dependencies.add("avplugin-57");
                        dependencies.add("avplugin-ffmpeg-56");
                        dependencies.add("avplugin-ffmpeg-57");
                    }

With the addition of the above, I can run media (although I get warnings...see my comment in NativeLibLoader).

@kevinrushforth

kevinrushforth Jun 13, 2018

Contributor

For Linux, the following are also needed as dependencies:

                    if (HostUtils.isLinux()) {
                        dependencies.add("fxplugins");
                        dependencies.add("avplugin-54");
                        dependencies.add("avplugin-56");
                        dependencies.add("avplugin-57");
                        dependencies.add("avplugin-ffmpeg-56");
                        dependencies.add("avplugin-ffmpeg-57");
                    }

With the addition of the above, I can run media (although I get warnings...see my comment in NativeLibLoader).

This comment has been minimized.

@saudet

saudet Jun 14, 2018

I don't see them in NativeLibLoader.java. What warnings would that be?

@saudet

saudet Jun 14, 2018

I don't see them in NativeLibLoader.java. What warnings would that be?

This comment has been minimized.

@johanvos

johanvos Jun 14, 2018

Collaborator

loading fxplugins as a library fails (with a warning), as it depends on libgstreamer-lite
ldd ~/.openjfx/cache/11-internal/libfxplugins.so
... libgstreamer-lite.so => not found
(it's in the same directory)

@johanvos

johanvos Jun 14, 2018

Collaborator

loading fxplugins as a library fails (with a warning), as it depends on libgstreamer-lite
ldd ~/.openjfx/cache/11-internal/libfxplugins.so
... libgstreamer-lite.so => not found
(it's in the same directory)

This comment has been minimized.

@johanvos

johanvos Jun 14, 2018

Collaborator

I added a boolean "load" to the loadLibraryFromResource method, which is true by default, but false when loading dependencies. In that case, the resource is simply unpacked.

@johanvos

johanvos Jun 14, 2018

Collaborator

I added a boolean "load" to the loadLibraryFromResource method, which is true by default, but false when loading dependencies. In that case, the resource is simply unpacked.

Show outdated Hide outdated ...s/javafx.graphics/src/main/java/com/sun/glass/utils/NativeLibLoader.java Outdated

johanvos added some commits Jun 27, 2018

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 27, 2018

Contributor

The two recent changes looks fine to me, and I can run two concurrent apps on Windows now.

Are there other changes you plan to make? I only have one final test I'd like to do before completing my review. If you are ready for final review prior to merging this fix, maybe you can drop the "[WIP]" from the title of the PR and send a review request to openjfx-dev?

Contributor

kevinrushforth commented Jun 27, 2018

The two recent changes looks fine to me, and I can run two concurrent apps on Windows now.

Are there other changes you plan to make? I only have one final test I'd like to do before completing my review. If you are ready for final review prior to merging this fix, maybe you can drop the "[WIP]" from the title of the PR and send a review request to openjfx-dev?

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 28, 2018

I see at least one more thing missing from this PR. It doesn't provide a locking mechanism to synchronize 2 or more JVM processes trying to modify the cache at the same time...

saudet commented Jun 28, 2018

I see at least one more thing missing from this PR. It doesn't provide a locking mechanism to synchronize 2 or more JVM processes trying to modify the cache at the same time...

@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 28, 2018

Collaborator

There are probably many edge cases that need to be fixed. I think we should create a JEP for this and target that to OpenJDK. Having it standardized in the JDK would be nicer than each project having to maintain a solution with all edge-cases.

Collaborator

johanvos commented Jun 28, 2018

There are probably many edge cases that need to be fixed. I think we should create a JEP for this and target that to OpenJDK. Having it standardized in the JDK would be nicer than each project having to maintain a solution with all edge-cases.

@johanvos johanvos changed the title from [WIP] build standalone modules (jars) to be uploaded to remote repositories to Build standalone modules (jars) to be uploaded to remote repositories Jun 28, 2018

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 28, 2018

Contributor

I did some additional testing and a 0 byte .so file is written in the case where the file exists but the checksum does not match. You might need to reopen or rewind the input stream after computing the checksum.

Contributor

kevinrushforth commented Jun 28, 2018

I did some additional testing and a 0 byte .so file is written in the case where the file exists but the checksum does not match. You might need to reopen or rewind the input stream after computing the checksum.

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 29, 2018

@johanvos Yes, exactly my point! But the guys at Oracle are not interested, so a JEP is pointless for now. Unless you have a better idea, I'll just keep trying to make a point with JavaCPP... @kevinrushforth or anyone else, tips welcome as to how to go about this.

saudet commented Jun 29, 2018

@johanvos Yes, exactly my point! But the guys at Oracle are not interested, so a JEP is pointless for now. Unless you have a better idea, I'll just keep trying to make a point with JavaCPP... @kevinrushforth or anyone else, tips welcome as to how to go about this.

read inputstream (for digest processing) in bigger chunks.
In case digest is computed, inputstream is consumed.
InflaterInputStream doesn't support mark/reset so we have to recreate the inputstream
@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jun 29, 2018

Collaborator

the inputstream obtained from class.getResourceAsStream() doesn't support mark/reset so I have to recreate it, and slightly modified the signature of a method.
Tested it on linux now, by manually manipulating one library, and that works.
I also improved the read performance for the digestInputStream by using a 4K buffer.

Collaborator

johanvos commented Jun 29, 2018

the inputstream obtained from class.getResourceAsStream() doesn't support mark/reset so I have to recreate it, and slightly modified the signature of a method.
Tested it on linux now, by manually manipulating one library, and that works.
I also improved the read performance for the digestInputStream by using a 4K buffer.

@kevinrushforth kevinrushforth removed the WIP label Jun 29, 2018

@kevinrushforth

This comment has been minimized.

Show comment
Hide comment
@kevinrushforth

kevinrushforth Jun 29, 2018

Contributor

Latest changes look good. I have tested on all three platforms with the following cases:

  • file doesn't exist
  • file exists but checksum doesn't match
  • file exists and checked matches (verified that existing file is used without being modified)
Contributor

kevinrushforth commented Jun 29, 2018

Latest changes look good. I have tested on all three platforms with the following cases:

  • file doesn't exist
  • file exists but checksum doesn't match
  • file exists and checked matches (verified that existing file is used without being modified)
@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 30, 2018

In my tests I've found a buffer size of 64 K to be optimal, especially on Windows, which is really slow with small buffer sizes like 1 or 4 K, but a channel as used by JNR is probably best:
https://github.com/jnr/jffi/blob/master/src/main/java/com/kenai/jffi/internal/StubLoader.java#L367

saudet commented Jun 30, 2018

In my tests I've found a buffer size of 64 K to be optimal, especially on Windows, which is really slow with small buffer sizes like 1 or 4 K, but a channel as used by JNR is probably best:
https://github.com/jnr/jffi/blob/master/src/main/java/com/kenai/jffi/internal/StubLoader.java#L367

@saudet

This comment has been minimized.

Show comment
Hide comment
@saudet

saudet Jun 30, 2018

Reading up on the formalities of the OpenJDK, it sounds like we would need the OK from Mark Reinhold to have any chance of having this added to the JDK. If you guys could find out his thinking on this topic the next time you meet, it would be great! If he sounds interested by that kind of feature, then it would make sense to start drawing up a JEP. If he is not interested or dismisses any discussions about this, then finding out exactly why would be useful information to figure out a way to make him understand the importance of this...

saudet commented Jun 30, 2018

Reading up on the formalities of the OpenJDK, it sounds like we would need the OK from Mark Reinhold to have any chance of having this added to the JDK. If you guys could find out his thinking on this topic the next time you meet, it would be great! If he sounds interested by that kind of feature, then it would make sense to start drawing up a JEP. If he is not interested or dismisses any discussions about this, then finding out exactly why would be useful information to figure out a way to make him understand the importance of this...

johanvos added some commits Jul 11, 2018

simply javafx.pom
use RELEASE_VERSION as maven version
use maven artifactId conventions ("-" instead of ".")
@kevinrushforth

Whatever you decide about MAVEN_VERSION, I recommend adding the following log statement (after the similar one for RELEASE_VERSION_PADDED):

logger.quiet("MAVEN_VERSION: $MAVEN_VERSION")
@@ -552,6 +551,7 @@ if (HUDSON_JOB_NAME == "not_hudson") {
defineProperty("RELEASE_SUFFIX", relSuffix)
defineProperty("RELEASE_VERSION_SHORT", "${RELEASE_VERSION}${RELEASE_SUFFIX}")
defineProperty("RELEASE_VERSION_LONG", "${RELEASE_VERSION_SHORT}+${PROMOTED_BUILD_NUMBER}${relOpt}")
defineProperty("MAVEN_VERSION", "$RELEASE_VERSION_LONG")

This comment has been minimized.

@kevinrushforth

kevinrushforth Jul 12, 2018

Contributor

This will be "11-ea+19" for the next build. Will this work? Or does it need to be something more like "11.0.0-ea-19"?

Here are the settings that were used by our Hudson job for build 18:

PROMOTED_BUILD_NUMBER: 18
RELEASE_VERSION: 11
RELEASE_SUFFIX: -ea
RELEASE_VERSION_SHORT: 11-ea
RELEASE_VERSION_LONG: 11-ea+18
RELEASE_VERSION_PADDED: 11.0.0.0
@kevinrushforth

kevinrushforth Jul 12, 2018

Contributor

This will be "11-ea+19" for the next build. Will this work? Or does it need to be something more like "11.0.0-ea-19"?

Here are the settings that were used by our Hudson job for build 18:

PROMOTED_BUILD_NUMBER: 18
RELEASE_VERSION: 11
RELEASE_SUFFIX: -ea
RELEASE_VERSION_SHORT: 11-ea
RELEASE_VERSION_LONG: 11-ea+18
RELEASE_VERSION_PADDED: 11.0.0.0
Show outdated Hide outdated javafx.pom Outdated
@kevinrushforth

As long as you are happy with the MAVEN_VERSION setting, this looks ready to go in.

@kevinrushforth

latest change looks fine

@shruda

This comment has been minimized.

Show comment
Hide comment
@shruda

shruda Jul 13, 2018

@johanvos,
@kevinrushforth asked you some days ago about the empty jars but I found no answer at the existing comments.

We did some tests with your published SNAPSHOTS's and we had to force an exclude of the empty jars at the dependencies.
Otherwise e.g. Eclipse shows a warning that the module name is instable because of the "auto-generated" module name in case of the empty jars.

Best Regards,
Steve

shruda commented Jul 13, 2018

@johanvos,
@kevinrushforth asked you some days ago about the empty jars but I found no answer at the existing comments.

We did some tests with your published SNAPSHOTS's and we had to force an exclude of the empty jars at the dependencies.
Otherwise e.g. Eclipse shows a warning that the module name is instable because of the "auto-generated" module name in case of the empty jars.

Best Regards,
Steve

@tiainen

This comment has been minimized.

Show comment
Hide comment
@tiainen

tiainen Jul 13, 2018

Contributor

The empty jars are required because we'd like to support both maven and gradle. The base requirement is that we can declare dependencies between the javafx artifacts (i.e. javafx-controls depends on javafx-graphics, which in turn depends on javafx-base). This will allow maven and gradle to automatically download the correct artifacts.

Case 1

We remove the empty jars. We define the dependencies without specifying the platform classifier. This will cause maven to try and find a javafx-graphics jar without classifier which doesn't exist. We only have artifacts with a classifier. During dependency resolution, maven will fail with the following message.

[ERROR] Failed to execute goal on project hellosphere: Could not resolve dependencies for project org.projavafx.jfx3d:hellosphere:jar:1.0-SNAPSHOT: The following artifacts could not be resolved: org.openjfx:javafx-graphics:jar:11-internal+0-2018-07-13-144331, org.openjfx:javafx-base:jar:11-internal+0-2018-07-13-144331: Failure to find org.openjfx:javafx-graphics:jar:11-internal+0-2018-07-13-144331 in https://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced -> [Help 1]

Case 2

We remove the empty jars. To make it work in maven, we define the dependencies by adding the classifier element to each dependency. For maven, we define a property named ${javafx.platform} that gets filled in automatically by the parent javafx pom definition (see javafx.pom). This works fine in maven, but gradle doesn't process the pom files in the same way. It doesn't resolve the maven property and will therefor fail when resolving the dependencies with the following message:

* What went wrong:
Could not resolve all files for configuration ':compileClasspath'.
> Could not find javafx-graphics-${javafx.platform}.jar (org.openjfx:javafx-graphics:11-internal+0-2018-07-13-140316).
> Could not find javafx-base-${javafx.platform}.jar (org.openjfx:javafx-base:11-internal+0-2018-07-13-140316).

In the end, the solution was to introduce an empty jar file.

Contributor

tiainen commented Jul 13, 2018

The empty jars are required because we'd like to support both maven and gradle. The base requirement is that we can declare dependencies between the javafx artifacts (i.e. javafx-controls depends on javafx-graphics, which in turn depends on javafx-base). This will allow maven and gradle to automatically download the correct artifacts.

Case 1

We remove the empty jars. We define the dependencies without specifying the platform classifier. This will cause maven to try and find a javafx-graphics jar without classifier which doesn't exist. We only have artifacts with a classifier. During dependency resolution, maven will fail with the following message.

[ERROR] Failed to execute goal on project hellosphere: Could not resolve dependencies for project org.projavafx.jfx3d:hellosphere:jar:1.0-SNAPSHOT: The following artifacts could not be resolved: org.openjfx:javafx-graphics:jar:11-internal+0-2018-07-13-144331, org.openjfx:javafx-base:jar:11-internal+0-2018-07-13-144331: Failure to find org.openjfx:javafx-graphics:jar:11-internal+0-2018-07-13-144331 in https://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced -> [Help 1]

Case 2

We remove the empty jars. To make it work in maven, we define the dependencies by adding the classifier element to each dependency. For maven, we define a property named ${javafx.platform} that gets filled in automatically by the parent javafx pom definition (see javafx.pom). This works fine in maven, but gradle doesn't process the pom files in the same way. It doesn't resolve the maven property and will therefor fail when resolving the dependencies with the following message:

* What went wrong:
Could not resolve all files for configuration ':compileClasspath'.
> Could not find javafx-graphics-${javafx.platform}.jar (org.openjfx:javafx-graphics:11-internal+0-2018-07-13-140316).
> Could not find javafx-base-${javafx.platform}.jar (org.openjfx:javafx-base:11-internal+0-2018-07-13-140316).

In the end, the solution was to introduce an empty jar file.

@johanvos

This comment has been minimized.

Show comment
Hide comment
@johanvos

johanvos Jul 13, 2018

Collaborator

support for modules is still in flux with maven/gradle, so this will be a moving target, especially because there are so many combinations of {repositories, build systems, IDE's}.

I suggest we continue this discussion on the mailinglist instead of on this PR?

Collaborator

johanvos commented Jul 13, 2018

support for modules is still in flux with maven/gradle, so this will be a moving target, especially because there are so many combinations of {repositories, build systems, IDE's}.

I suggest we continue this discussion on the mailinglist instead of on this PR?

@johanvos johanvos merged commit c168ab5 into javafxports:develop Jul 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shruda

This comment has been minimized.

Show comment
Hide comment
@shruda

shruda Jul 13, 2018

Thanks @tiainen for the explanation.
@johanvos , it's okay for me.

shruda commented Jul 13, 2018

Thanks @tiainen for the explanation.
@johanvos , it's okay for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment