Add option to expand wildcard in entrypoint runtime classpath #2866

chanseokoh · 2020-10-26T22:23:09Z

Gives a viable solution to #1871 (#1907, #2228, #2733).

Also enables the AppCDS support (#2471).

This PR adds the option jib.container.expandClasspathDependencies that expands the wildcard .../libs/* in the entrypoint JVM classpath. It's off by default. The feature to use an @argument file will be added later for a permanent fix for the issue later. See #2866 (comment) for details.

loosebazooka · 2020-10-27T06:04:56Z

jib-core/src/main/java/com/google/cloud/tools/jib/global/JibSystemProperties.java

  @VisibleForTesting public static final String SKIP_EXISTING_IMAGES = "jib.skipExistingImages";

+  @VisibleForTesting
+  static final String USE_WILDCARD_FOR_DEPENDENCY_CLASSPATH =
+      "jib.useWildcardForDependencyClasspath";


I realize this is a draft, but I'm not super fond of this name. I sometimes think preserveClasspathOrder is a decent name, but i'm not sure.

Sounds good. And this made me realize that JibSystemProperties is for jib-core. The property should be in jib-plugins-common.

I think it's natural to negate the name, such as noClasspathOrderPreserving. The PropertyNames class only defines property names, and Boolean.getBoolean() doesn't have a way to return true as default (which I think makes sense to avoid confusion).

Hrmm.. maybe in that case, your naming makes more sense. useClasspathWildcard is similar but shorter?

Also any idea if we would include this as a config option rather than system property?

Actually maybe the full name is necessary

The idea was to have the flag as a temporary escape hatch that will go away soon once we don't get any report from the user. But I think I was too naive. Previous I speculated that we don't have to worry about the argument length limit (#2733 (comment), #2471 (comment)). However, after searching "argument list too long" on the Internet today, I completely lost my previous confidence. Now I think the length limit is real and this change is not safe. For example: gradle/gradle#5754.

So it would be safer to have this as an opt-in feature. But that's really really unfortunate, because issues from a different library order are very cryptic and difficult to identify the cause. I really wish Java 8 supported the @-argument file feature.

Anyways, if we do this, now I think we should have it as a config option (and additionally a system property). But the question as to whether we expand it by default or not remains.

But ideally, for Java 9+, I think we should really use the @-argument file feature; we can always preserve the classpath order while never hitting the system argument length limit. This is going to be a lot bigger change.

In sum, perhaps

Java 9+: always use a @-argument file. (preserveClasspathOrder is irrelevant.)

Java <=8: expand or not based on preserveClasspathOrder. Not decisive on if it should be true by default.

I think false by default works (as it is the current behavior) for java8-

chanseokoh · 2020-11-02T16:57:37Z

Now out of draft.

Renamed and relocated the flag to jib.container.expandClasspathDependencies (affects the entrypoint container config). Made the default to false (keeping the current behavior to use .../libs/*.

I think this feature can go in independently of using the @argument file.

Need to update the public usage doc after we release 2.7.0.

mpeddada1 · 2020-11-02T18:41:24Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenProjectProperties.java

+    return project
+        .getArtifacts()
+        .stream()
+        .map(artifact -> artifact.getFile().toPath())


GradleProjectProperties also checks if the dependency is a jar whereas this does not. Is this intentional?

Good point. I am not sure if all the artifacts from project.getArtifacts() will always be .jar files, but I think usually they will be .jar files for typical Java projects.

I've checked the MavenProjectProperties code gain, and I can tell at least that we always put all the files from project.getArtifacts() into /app/libs. (All of them are added as dependencies and only them.)

jib/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenProjectProperties.java

Lines 291 to 299 in a720aa1

Map<LayerType, List<Path>> classifiedDependencies =

classifyDependencies(project.getArtifacts(), getProjectDependencies());

javaContainerBuilder.addDependencies(

Preconditions.checkNotNull(classifiedDependencies.get(LayerType.DEPENDENCIES)));

javaContainerBuilder.addSnapshotDependencies(

Preconditions.checkNotNull(classifiedDependencies.get(LayerType.SNAPSHOT_DEPENDENCIES)));

javaContainerBuilder.addProjectDependencies(

Preconditions.checkNotNull(classifiedDependencies.get(LayerType.PROJECT_DEPENDENCIES)));

(classifyDependencies(...) classifies project.getArtifacts() into project, snapshot, and third-party dependencies.)

So, doing this should match the actual dependencies we put.

And for Gradle, checking .jar is a bit different behavior to obtain dependencies than we current do in GradleProjectProperties.createJibContainerBuilder(); it doesn't strictly follow the logic there. However, I think this should work.

mpeddada1

I'm guessing that since the default is now set to current behavior, length restriction for the classpath shouldn't be as big of a problem? Should we still document this as a warning when the user chooses to enable classpath expansion.

chanseokoh · 2020-11-02T19:04:42Z

I think we should shorten the classpath, because we show the entrypoint by default and it will always be extremely long (lie, super super long) whenever the user turns on the option.

chanseokoh · 2020-11-02T19:06:00Z

About the warning, I think you're talking about a potential failure due to line length limit? That's a good idea. I'll document that in CHANGELOG. We should also document it when we update our usage doc.

chanseokoh · 2020-11-03T16:09:06Z

Manual testing works correctly. Merging now.

DamianDunajski · 2020-12-02T15:42:38Z

Great work 👍 The feature solved my issue in quite a few Spring apps.

Is there any plan to release it?

chanseokoh · 2020-12-02T15:44:24Z

We plan to make a release very soon. Will let you know.

chanseokoh · 2020-12-07T17:35:39Z

@geek-soft Jib 2.7.0 has been released with this feature! Let us know if you hit any issues.

Expand wildcard in entrypoint runtime classpath

17308cb

google-cla bot added the cla: yes label Oct 26, 2020

loosebazooka reviewed Oct 27, 2020

View reviewed changes

chanseokoh added 2 commits October 27, 2020 10:37

Update CHANGELOG

022de85

Update and migrate system property

86042e8

This was referenced Oct 27, 2020

Classpath ordering in container dependent on host filesystem #2733

Closed

Enable to generate and package a CDS archive #2471

Open

Rename config to "expandClasspathDependencies" and flip default

2881777

chanseokoh requested a review from mpeddada1 November 2, 2020 16:59

chanseokoh marked this pull request as ready for review November 2, 2020 16:59

chanseokoh changed the title ~~(Draft) Expand wildcard in entrypoint runtime classpath~~ Add option to expand wildcard in entrypoint runtime classpath Nov 2, 2020

mpeddada1 reviewed Nov 2, 2020

View reviewed changes

Document potential failure in CHANGELOG

880124d

mpeddada1 approved these changes Nov 2, 2020

View reviewed changes

mpeddada1 added this to the v2.7.0 milestone Nov 2, 2020

chanseokoh merged commit f0123a8 into master Nov 3, 2020

chanseokoh deleted the expanded-classpath branch November 3, 2020 16:09

chanseokoh mentioned this pull request Dec 7, 2020

Jar loading order error causes spring-boot startup to fail #1871

Closed

chanseokoh mentioned this pull request Jun 4, 2021

Java 9 long classpath support to enable deterministic class loading order #3280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to expand wildcard in entrypoint runtime classpath #2866

Add option to expand wildcard in entrypoint runtime classpath #2866

chanseokoh commented Oct 26, 2020 •

edited

Loading

loosebazooka Oct 27, 2020

chanseokoh Oct 27, 2020

chanseokoh Oct 27, 2020 •

edited

Loading

loosebazooka Oct 27, 2020

loosebazooka Oct 27, 2020

chanseokoh Oct 27, 2020 •

edited

Loading

loosebazooka Oct 27, 2020

chanseokoh commented Nov 2, 2020

mpeddada1 Nov 2, 2020

chanseokoh Nov 2, 2020

chanseokoh Nov 2, 2020 •

edited

Loading

mpeddada1 left a comment

chanseokoh commented Nov 2, 2020

chanseokoh commented Nov 2, 2020

chanseokoh commented Nov 3, 2020

DamianDunajski commented Dec 2, 2020

chanseokoh commented Dec 2, 2020

chanseokoh commented Dec 7, 2020

	Map<LayerType, List<Path>> classifiedDependencies =
	classifyDependencies(project.getArtifacts(), getProjectDependencies());

	javaContainerBuilder.addDependencies(
	Preconditions.checkNotNull(classifiedDependencies.get(LayerType.DEPENDENCIES)));
	javaContainerBuilder.addSnapshotDependencies(
	Preconditions.checkNotNull(classifiedDependencies.get(LayerType.SNAPSHOT_DEPENDENCIES)));
	javaContainerBuilder.addProjectDependencies(
	Preconditions.checkNotNull(classifiedDependencies.get(LayerType.PROJECT_DEPENDENCIES)));

Add option to expand wildcard in entrypoint runtime classpath #2866

Add option to expand wildcard in entrypoint runtime classpath #2866

Conversation

chanseokoh commented Oct 26, 2020 • edited Loading

loosebazooka Oct 27, 2020

Choose a reason for hiding this comment

chanseokoh Oct 27, 2020

Choose a reason for hiding this comment

chanseokoh Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

loosebazooka Oct 27, 2020

Choose a reason for hiding this comment

loosebazooka Oct 27, 2020

Choose a reason for hiding this comment

chanseokoh Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

loosebazooka Oct 27, 2020

Choose a reason for hiding this comment

chanseokoh commented Nov 2, 2020

mpeddada1 Nov 2, 2020

Choose a reason for hiding this comment

chanseokoh Nov 2, 2020

Choose a reason for hiding this comment

chanseokoh Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

mpeddada1 left a comment

Choose a reason for hiding this comment

chanseokoh commented Nov 2, 2020

chanseokoh commented Nov 2, 2020

chanseokoh commented Nov 3, 2020

DamianDunajski commented Dec 2, 2020

chanseokoh commented Dec 2, 2020

chanseokoh commented Dec 7, 2020

chanseokoh commented Oct 26, 2020 •

edited

Loading

chanseokoh Oct 27, 2020 •

edited

Loading

chanseokoh Oct 27, 2020 •

edited

Loading

chanseokoh Nov 2, 2020 •

edited

Loading