-
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
Add runtimeOnly dependencies to quarkusDev classpath #18139
Conversation
@@ -335,7 +335,7 @@ private void addSelfWithLocalDeps(Project project, GradleDevModeLauncher.Builder | |||
if (!visited.add(project.getPath())) { | |||
return; | |||
} | |||
final Configuration compileCp = project.getConfigurations().findByName(JavaPlugin.COMPILE_CLASSPATH_CONFIGURATION_NAME); | |||
final Configuration compileCp = project.getConfigurations().findByName(JavaPlugin.RUNTIME_CLASSPATH_CONFIGURATION_NAME); |
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.
There could also be a compile-only config which is relevant in dev mode, e.g. lombok. Could you check please?
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 try adding lombok to the reproducer. The dev mode starts correctly, and if I update the class (e.g adding @log) it works too.
Doesn't the dev mode rely on the model to create a compile classpath that is used to recompile updated 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.
Sure. In this method we are determining locally available projects that should be hot-reloadable. So if there is compileOnly dependency on a local project, changes to that project won't be triggering a reload. So, lombok isn't a good test in this case. You see what I mean?
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 see, it looks completely logical, I will update the branch
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.
@aloubyansky I updated the code to take care of compileOnly
dependencies.
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building ef0bedf
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 #📦 integration-tests/gradle✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ |
@@ -335,7 +335,9 @@ private void addSelfWithLocalDeps(Project project, GradleDevModeLauncher.Builder | |||
if (!visited.add(project.getPath())) { | |||
return; | |||
} | |||
final Configuration compileCp = project.getConfigurations().findByName(JavaPlugin.COMPILE_CLASSPATH_CONFIGURATION_NAME); | |||
final Configuration compileCp = project.getConfigurations().create("compileCp") |
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.
To be consistent we could use QuarkusPlugin.DEV_MODE_CONFIGURATION_NAME, right?
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 looks like it should already be available?
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 and it will also include ˋquarkusDev dependency. This configuration only includes runtime classpath plus ˋcompileOnly
artifacts. I will add runtimeOnly
artifacts too.
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 5b36fed
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 #📦 integration-tests/gradle✖ ⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖ |
@aloubyansky I updated the code, and the build is successful, is it ok for you? |
@@ -204,7 +204,8 @@ public void execute(Task test) { | |||
// create a custom configuration for devmode | |||
configurations.create(DEV_MODE_CONFIGURATION_NAME).extendsFrom( | |||
configurations.getByName(JavaPlugin.COMPILE_ONLY_CONFIGURATION_NAME), | |||
configurations.getByName(JavaPlugin.RUNTIME_CLASSPATH_CONFIGURATION_NAME)); | |||
configurations.getByName(JavaPlugin.RUNTIME_CLASSPATH_CONFIGURATION_NAME), | |||
configurations.getByName(JavaPlugin.RUNTIME_ONLY_CONFIGURATION_NAME)); |
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.
Shouldn't RUNTIM_CLASSPATH include RUNTIME_ONLY?
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, this works, I pushed the update.
🚫 This workflow run has been cancelled. ✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself. |
@gsmet I waited for the CI to be done before merging, but the bot reported the job as |
This adds runtime dependencies to module classpath for devmode.
close #18117