-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 option to expand wildcard in entrypoint runtime classpath #2866
Conversation
@VisibleForTesting public static final String SKIP_EXISTING_IMAGES = "jib.skipExistingImages"; | ||
|
||
@VisibleForTesting | ||
static final String USE_WILDCARD_FOR_DEPENDENCY_CLASSPATH = | ||
"jib.useWildcardForDependencyClasspath"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. And this made me realize that JibSystemProperties
is for jib-core. The property should be in jib-plugins-common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually maybe the full name is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think false by default works (as it is the current behavior) for java8-
Now out of draft. Renamed and relocated the flag to I think this feature can go in independently of using the Need to update the public usage doc after we release 2.7.0. |
return project | ||
.getArtifacts() | ||
.stream() | ||
.map(artifact -> artifact.getFile().toPath()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GradleProjectProperties
also checks if the dependency is a jar whereas this does not. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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 |
Manual testing works correctly. Merging now. |
Great work 👍 The feature solved my issue in quite a few Spring apps. Is there any plan to release it? |
We plan to make a release very soon. Will let you know. |
@geek-soft Jib 2.7.0 has been released with this feature! Let us know if you hit any issues. |
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.