-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support for project modules that produce multiple JARs (with classifiers) #22336
Conversation
@glefloch this should allow us to better represent the Gradle model too, although it's not yet fully leveraged. |
Great, I'll put this high on my list! |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 646c0f3
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/jdbc/jdbc-mssql/deployment
! Skipped: integration-tests/jpa-mssql 📦 extensions/jdbc/jdbc-mssql/deployment✖
⚙️ Native Tests - HTTP #- Failing: integration-tests/vertx-http
📦 integration-tests/vertx-http✖
|
@aloubyansky mvn clean compile quarkus:dev force restart (s) |
Wow. Thanks a lot @Postremus! |
@Postremus what did you use to profile bootstrapping? I am going to look into it. |
@aloubyansky For bootstrap profiling on windows, I use following approach:
On Linux / wslv2, I use async-profiler directly.
|
Thanks! |
Just fyi, I found how to get it to pretty much the same numbers as in 2.6.0.Final. I'll push an update once I clean my new changes up. |
@aloubyansky cool, I'll start testing once you have pushed that iteration. |
646c0f3
to
6d572df
Compare
I pushed more changes with optimizations. Here is what I collected from 10 launches of
So the boot is still slower (especially compared to main - great job @Postremus), although not as dramatically. The restart was consistently faster by a bit, however. |
I'll need to fix this on Windows, probably a path handling issue
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 6d572df
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/smallrye-openapi/deployment
! Skipped: extensions/agroal/deployment extensions/elytron-security-jdbc/deployment extensions/flyway/deployment and 133 more 📦 extensions/smallrye-openapi/deployment✖
⚙️ Native Tests - HTTP #- Failing: integration-tests/vertx-http
📦 integration-tests/vertx-http✖
⚙️ Native Tests - gRPC #- Failing: integration-tests/grpc-mutual-auth integration-tests/grpc-plain-text-mutiny
📦 integration-tests/grpc-mutual-auth✖
✖
✖
✖
📦 integration-tests/grpc-plain-text-mutiny✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
|
I think we should get to the bottom of the slowdown because I fail to see how a change to support producing multiple jars could impact the default case. |
Looks like @stuartwdouglas is going to be on vacation until the 17th and might not have time to review this until the 2.7.0.CR1 (19.01), in which case it may be too late to include this in 2.7.0.Final (02.02). |
Bummer, but thanks for letting us know. |
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 tried to be of some help and reviewed what I felt able to (and left some small comments).
LGTM overall, but I'm lacking in-depth knowledge of some parts!
Files.walkFileTree(root, new SimpleFileVisitor<Path>() { | ||
@Override | ||
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { | ||
if (dir.equals(root) || dir.toString().equals("/") || dir.startsWith(devTemplatesPath)) |
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.
Is this intentionally missing the curly brackets? This class seems a bit inconsistent in that regard, even before your changes.
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 didn't pay attention, tbh. I'll add the brackets.
|
||
PathCollection getBuildFiles(); | ||
|
||
default PathTree getContentTree(String classifier) { | ||
//System.out.println("WorkspaceModule.getContentTree " + /* getId() + */ ":" + classifier); |
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.
This can probably be removed now?
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.
Yes, I'll remove it, I kept it as a reminder this line could break dev mode.
try { | ||
super.close(); | ||
} finally { | ||
fs.close(); |
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.
No idea how realistic either of the cases is here and maybe it was a deliberate choice, but if this fails it would shadow the exception from super.close()
(if any).
Maybe add a catch
and use addSuppressed()
?
|
||
@Override | ||
public OpenPathTree openTree() { | ||
return this;//CachingPathTree.of(this); |
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.
Is //CachingPathTree.of(this);
a leftover?
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.
It's kind of a WIP. I'll remove it.
List<Integer> list = (List<Integer>) v.getClass().getMethod("version").invoke(v); | ||
version = list.get(0); | ||
} catch (Exception e) { | ||
//version 8 |
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.
Do we even support Java 8 anymore? pom.xml says no if I'm not mistaken.
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.
We don't. I copied this code from another class, tbh. I'll change it to fail hard.
try { | ||
return new Manifest(new ByteArrayInputStream(mf.getData())); | ||
} catch (IOException e) { | ||
log.warnf("Failed to parse manifest for %s", toString()); |
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.
Is this intentionally swallowing the IOE?
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'll add e
to the warnf
.
@Override | ||
public Set<String> getProvidedResources() { | ||
if (resources == null) { | ||
synchronized (this) { |
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 probably missing something important or obvious, but shouldn't this use the locks instead?
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.
@Override | ||
protected Manifest readManifest() { | ||
return withOpenTree(tree -> { | ||
return tree.getManifest(); |
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.
Could be a method ref instead, AFAICS.
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.
Right.
list.add(include.getValue()); | ||
} | ||
filter = PathFilter.forIncludes(list); | ||
} else { |
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.
Are you sure about ignoring excludes when there are includes?
E.g.: https://stackoverflow.com/a/2450803/9529981
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.
No, I'll fix it.
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.
Btw, this is a working link to that Scanner class: https://codehaus-plexus.github.io/plexus-utils/apidocs/org/codehaus/plexus/util/DirectoryScanner.html
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.
Thanks, we'll need to add proper tests for this.
|
||
return new FilePathTree(p); | ||
} catch (IOException e) { | ||
throw new IllegalArgumentException(p + " does not exist"); |
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.
This is probably the most likely case, but javadoc of Files.readAttributes
says: if an I/O error occurs
, so better include the IOE as the cause?
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.
Agreed
f0aa871
to
c0dc00f
Compare
c0dc00f
to
8e83b6c
Compare
@aloubyansky I was going to merge but unfortunately there is a conflict. |
8e83b6c
to
0f37bb0
Compare
Yes, rebased. |
OK, so let's merge this but it's big so let's be ready to fix things quickly after the CR1 release if things go south for some weird project setups. |
7856f6a
to
db58b6b
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building db58b6b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/webjars-locator/deployment
! Skipped: integration-tests/webjars-locator 📦 extensions/webjars-locator/deployment✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/webjars-locator/deployment
! Skipped: integration-tests/webjars-locator 📦 extensions/webjars-locator/deployment✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/webjars-locator/deployment
! Skipped: integration-tests/webjars-locator 📦 extensions/webjars-locator/deployment✖
|
588faf2
to
90b63db
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 90b63db
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/vertx-http/deployment
! Skipped: core/test-extension/deployment extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 290 more 📦 extensions/vertx-http/deployment✖ ⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/resteasy-classic/resteasy/deployment
! Skipped: extensions/agroal/deployment extensions/apicurio-registry-avro/deployment extensions/avro/deployment and 273 more 📦 extensions/resteasy-classic/resteasy/deployment✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/resteasy-classic/resteasy/deployment
! Skipped: extensions/agroal/deployment extensions/apicurio-registry-avro/deployment extensions/avro/deployment and 273 more 📦 extensions/resteasy-classic/resteasy/deployment✖
⚙️ Maven Tests - JDK 11 #- Failing: integration-tests/maven
📦 integration-tests/maven✖
✖
⚙️ Maven Tests - JDK 11 Windows #- Failing: integration-tests/maven
📦 integration-tests/maven✖
✖
|
90b63db
to
8bb2afd
Compare
This change allows to have a "view" of an artifact content located at one or more output directories applying include and exclude filters. Essentially, it adds a possibility to support project modules that produce more than one (JAR) artifact.
Currently, the assumption in dev and test modes is that if an artifact resolves to one or more output directories, all the content found in those directories will represent the artifact's content. If however there were include/exclude filters configured for the artifact, this may lead to more classes/resources being indexed and loaded, which may lead to errors. This PR is an attempt to support these cases.
There are quite a few changes in the dependency-related API, for example:
ResolvedDependency.getResolvedPaths()
now returns the paths that would have been resolved by Maven/Gradle (the current implementation prefers local output dirs in dev and test modes)ResolvedDependency.getContentTree()
returns an instance ofPathTree
representing artifact's content (whether it's a JAR or an output directory with possible filtering applied)ArtifactSources WorkspaceModule.getSources(String classifier)
allows to obtain the sources/output content layout for a given artifact (there are also helper methods likeArtifactSources getMainSources()
,ArtifactSources getTestSources()
)PathTreeClassPathElement
was introduced and became the main impl of theClassPathElement
@famod this should help your use-case, with this change your
mocks
classifier based ontest-jar
will be supported in dev mode. I would appreciate if you could give it a try and confirm it works for you. I did add a test in this PR for your use-case.@Postremus do you mind checking this PR and perhaps see how far it throws back the performance gains you've achieved lately? :)