-
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 running the same application multiple times in dev mode #22743
Conversation
Handler<RoutingContext> handler = recorder.handler(finalDestinationBuildItem.getSwaggerUiFinalDestination(), | ||
finalDestinationBuildItem.getSwaggerUiPath(), | ||
String swaggerUiPath = nonApplicationRootPathBuildItem.resolvePath(swaggerUiConfig.path); | ||
swaggerUiBuildProducer.produce(new SwaggerUiBuildItem(result.getFinalDestination(), swaggerUiPath)); |
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.
SwaggerUiBuildItem
is produced just to be on the safe side, no idea if anyone consumes it.
Maybe we can deprecate the build item as well?
smallRyeHealthBuildItem.getHealthUiPath(), runtimeConfig); | ||
String healthUiPath = nonApplicationRootPathBuildItem.resolvePath(healthConfig.ui.rootPath); | ||
smallryeHealthBuildProducer | ||
.produce(new SmallRyeHealthBuildItem(result.getFinalDestination(), healthUiPath)); |
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.
SmallRyeHealthBuildItem
is produced just to be on the safe side, no idea if anyone consumes it.
Maybe we can deprecate the build item as well?
8c17e36
to
f5ed857
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f5ed857
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
|
This PR is ready for review - it contains everything I wanted to include for now. Dev mode startup time worsens by about 30ms. According to profiling, this is because the temp directory is now deleted on shutdown, and webjar resources need to be copied again. I plan to open a follow up PR once this one is merged which will implement #22525 for swagger-ui, smallrye-health, graphql. |
Handler<RoutingContext> handler = recorder.uiHandler(smallRyeGraphQLBuildItem.getGraphqlUiFinalDestination(), | ||
smallRyeGraphQLBuildItem.getGraphqlUiPath(), runtimeConfig); | ||
String graphQLUiPath = nonApplicationRootPathBuildItem.resolvePath(graphQLConfig.ui.rootPath); | ||
smallRyeGraphQLBuildProducer |
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.
SmallRyeGraphQLBuildItem
is produced just to be on the safe side, no idea if anyone consumes it.
Maybe we can deprecate the build item as well?
|
||
ClassLoader classLoader = WebJarUtil.class.getClassLoader(); | ||
|
||
for (Path p : resourcesArtifact.getResolvedPaths()) { |
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 should be moving away from using resolved path to explore the content of a resolved dependency switching to PathTree userApplication.getContentTree()
. Then you walk the tree or process a specific path.
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.
done
String artifactId) { | ||
for (ResolvedDependency dep : curateOutcomeBuildItem.getApplicationModel().getDependencies()) { | ||
if (dep.getArtifactId().equals(artifactId) | ||
&& dep.getGroupId().equals(groupId)) { |
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.
Keep in mind, it may happen there could be multiple artifacts on the classpath with the same G:A but different classifiers.
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 now also compare the classifier in this place.
I saw no other way to compare a GACT to the GACTV of the ResolvedDependency.
|
||
resourcesArtifact.getContentTree().accept(webJar.getRoot(), new Consumer<PathVisit>() { | ||
@Override | ||
public void accept(PathVisit pathVisit) { |
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.
Theoretically, the passed in pathVisit
could be null if the content tree does not contain the path (webJar.getRoot), not sure if that could happen in this specific case though. I thought it would be up to the impl of the consumer whether to throw an error or simply return in such a case.
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.
done
for (ResolvedDependency dep : curateOutcomeBuildItem.getApplicationModel().getDependencies()) { | ||
if (dep.getArtifactId().equals(artifactKey.getArtifactId()) | ||
&& dep.getGroupId().equals(artifactKey.getGroupId()) | ||
&& dep.getClassifier().equals(artifactKey.getClassifier())) { |
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.
That or dep.getKey().equals(artifactKey)
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 thought about it, too, but I don't see how .equals
can return true.
ResolvedDependency.getKey()
returns an instance of GACTV.
We only pass a GACT in here, since the version of the webjar is not static.
E.g. io.quarkus: quarkus-vertx-http-deployment is one of 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.
I thought about it, too, but I don't see how
.equals
can return true.ResolvedDependency.getKey()
returns an instance of GACTV.
No, it returns an ArtifactKey
, which is GACT
.
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.
yeah, you are obviously right. I guess I got confused, because ResolvedArtifactDependency extends GACTV. :D
I cleaned this up a bit further, and implemented following additonal changes / fixes:
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 2455ecd
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
|
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.
Finally had the time to look at this one. I also migrated the Quarkus GitHub App to it to make sure things would be smooth.
Things are indeed far better with this setup and I was able to remove quite some boilerplate that was very ad hoc.
So it's a big +1 from me but... I added a few suggestions. They are open to discussion so feel free to push back :).
import io.quarkus.maven.dependency.ArtifactKey; | ||
import io.quarkus.maven.dependency.ResolvedDependency; | ||
|
||
public final class WebJarResultBuildItem extends SimpleBuildItem { |
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 we have some class-level javadoc here?
private static final GACT DEVCONSOLE_WEBJAR_ARTIFACT_KEY = new GACT("io.quarkus", "quarkus-vertx-http-deployment", null, | ||
"jar"); | ||
private static final String DEVCONSOLE_WEBJAR_STATIC_RESOURCES_PATH = "dev-static/"; | ||
private static final String DEVCONSOLE_FINAL_DESTINATION = "META-INF/dev-ui-files"; |
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.
Why do we need this now? Couldn't it be generated from the artifact id/version + resource path? That would simplify things a bit.
At least I don't think we should make it mandatory.
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 removed this option completly. Automaticly generating the final destination also prevents clashes between different webjars, which is quite nice.
import io.quarkus.maven.dependency.ArtifactKey; | ||
import io.quarkus.maven.dependency.ResolvedDependency; | ||
|
||
public final class WebJarResultBuildItem extends SimpleBuildItem { |
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 I would call it WebJarResultsBuildItem
plural. You have several and it would be less confusing.
this(artifactKey, webjarRoot, true, false, finalDestination, filter); | ||
} | ||
|
||
public WebJarBuildItem(GACT webJarKey, String webjarRoot, boolean useDefaultQuarkusBranding, |
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 would be handy to have versions of both constructors without the filter as it's purely optional.
@@ -44,7 +44,10 @@ | |||
|
|||
/** | |||
* Utility for Web resource related operations | |||
* | |||
* @deprecated Use WebJarBuildItem and WebJarResultBuildItem 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.
If you rename WebJarResultBuildItem
, make sure this is done everywhere including here.
private final boolean onlyCopyNonArtifactFiles; | ||
|
||
/** | ||
* Defines whether quarkus can override resources of the webjar with quarkus internal files. |
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.
* Defines whether quarkus can override resources of the webjar with quarkus internal files. | |
* Defines whether Quarkus can override resources of the webjar with Quarkus internal files. |
this(artifactKey, webjarRoot, true, false, finalDestination, filter); | ||
} | ||
|
||
public WebJarBuildItem(GACT webJarKey, String webjarRoot, boolean useDefaultQuarkusBranding, |
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.
That being said, I wonder if we should use a Builder approach for this one?
this(artifactKey, webjarRoot, true, false, finalDestination, filter); | ||
} | ||
|
||
public WebJarBuildItem(GACT webJarKey, String webjarRoot, boolean useDefaultQuarkusBranding, |
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.
Maybe rename webJarKey
to artifactKey
for consistency?
Thank you for the review @gsmet. |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f9f1334
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ JVM Tests - JDK 17 #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ Native Tests - Amazon #- Failing: integration-tests/amazon-lambda-http
📦 integration-tests/amazon-lambda-http✖
⚙️ Native Tests - Main #- Failing: integration-tests/main
📦 integration-tests/main✖
|
test failures look related, will have to take a closer look. |
@Postremus just a note that the cutoff for 2.7.0.CR1 is tonight and I would prefer having this one in CR1 given how structural it is. |
Should be fixed now, lets see what CI has to say. |
And all green! Thanks for your efforts on this @Postremus, it's much cleaner! |
Adds support for running the same application multiple times in dev mode on the same machine.
Problem was, that resources of webjars (like devui) where unpacked to %TEMP%/quarkus/applicationName/libraryName/libraryVersion.
On windows, this worked fine. %TEMP% points to a path inside the user directory (e.g. c:/users/martin/appdata/local/temp/).
On linux however, this caused AccessDeniedException, since %TEMP% points to a global temp directory, e.g. /temp.
This branch changes the location where webjars are unpacked to.
Now, every quarkus dev mode isntace creates a new temp directory, e.g. C:\Users\Martin\AppData\Local\Temp\quarkus-webjar10464518099488497426.
These temp directories are removed on shutdown.
The directory layout inside the temp directory is changed. It is now flatter, and the same as in the generated resource. e.g. quarkus-webjar10464518099488497426\META-INF\swagger-ui-files\index.html
I also deprecated io.quarkus.deployment.util.WebJarUtil (forRemoval) in favor of two new BuildItems - WebJarBuildItem and WebJarResultBuildItem.
WebJarBuildItem contains the instructions on which artifact should be unpacked and to which final destination (e.g. META-INF/swagger-ui-files). The webjar is either unpacked to disk (dev mode), or resources are generated (prod).
WebJarResultBuildItem contains the path where the webjar was unpacked to.
dev-ui, swagger-ui, smallrye-health and graphql already use these new builditems - in quarkus, no place uses the deprecated WebJarUtil anymore.
fixes #22515
Related zulip discussion: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Refactoring.20WebJarUtil