-
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 support of several resource and test resource folders in dev mode #17184
Conversation
76eddb4
to
87d99fc
Compare
if (Files.isDirectory(testResourcesSourcesDir)) { | ||
testResourcePath = testResourcesSourcesDir.toAbsolutePath().toString(); | ||
resourcePaths = localProject.getResourcesSourcesDirs().stream() | ||
.filter(Files::isDirectory) |
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 kept this filter to match with what was done before but do we rely need it? This will filter out non existing folders that could be added later while developing using the dev mode.
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 a better test could be notExists || isDirectory
?
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 not 100% sure but that may have been the intention to filter out dirs that don't 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.
If so it could be interesting to know why because in dev mode, you could want to add a new resource folder in the pom which would trigger an application restart then add a file in this new resource folder which won't be taken into account because of this filter (It is actually the scenario that I had in mind initially for the IT). Please also note that it seems to be inconsistent with Gradle according to what I understand from this part of the code, indeed it doesn't seem to apply any filter.
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.
When a POM changes we re-calculate the model anyway. I think it should still work.
The Gradle impl will need to be fixed.
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 would depend on the order, it would only work if you created the directory then added it to the pom.
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 would depend on the order, it would only work if you created the directory then added it to the pom.
I confirm that it only works this way, if you modify the pom then create the directory it doesn't work that is why I was wondering if we could simply remove this filter because if we do it will work whatever the order.
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 so what do you want me to do finally ? Keep this filter as it is and add it to the gradle part or remove 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 guess if it doesn't break anything it can be removed. I guess a comment about this use-case could be useful.
@@ -0,0 +1 @@ | |||
# Just to be able to commit this empty folder needed for the test |
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 should not be needed if we don't filter out missing directories
87d99fc
to
031630f
Compare
59ca8cc
to
baf154c
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 59ca8cc
|
baf154c
to
a16ece5
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a16ece5
Full information is available in the Build summary check run. Test Failures⚙️ MicroProfile TCKs Tests #📦 tcks/microprofile-fault-tolerance✖ |
The build failure doesn't seem to be related to this PR since the tck passes locally. Maybe it is a random failure |
Hi @aloubyansky and/or @stuartwdouglas WDYT of this PR? |
@@ -228,20 +230,28 @@ public Path getSourcesDir() { | |||
return getSourcesSourcesDir().getParent(); | |||
} | |||
|
|||
public Path getResourcesSourcesDir() { | |||
public Set<Path> getResourcesSourcesDirs() { |
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.
About using Set
s here and a few other places that represent resources of a single module, these paths will be used to compose a classpath, so it make it consistent, shouldn't we always use List
s instead? Is there really a high chance of finding duplicates in 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 can change it for List
everywhere if needed but I decided to use Set
instead because:
- I realized that in many places in this part of the code,
Set
s are used when a collection is needed like for example inSourceSet
/SourceSetImpl
orsourcePaths
inDevMode
- We could have duplicates for source directories for example when we use filtering and we would like to enable filtering for specific files only like this example
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.
Now I'm wondering why we are using Set<File>
in SourceSet
when we already have https://github.com/quarkusio/quarkus/blob/main/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/bootstrap/model/PathsCollection.java
Which could be enhanced to not add duplicates. While duplicate paths are possible I guess that'll still be rear. Another guess is that it's also unlikely we'll have to deal with more than a few resources dirs, in which case lists are more efficient than sets even for contains(item)
.
@stuartwdouglas do you see a problem switching to a list-based collection?
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 we should just define PathsCollection
as an ordered collection of paths that does not contain duplicates and use it everywhere.
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 should probably also be renamed to PathCollection
.
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 they are in a separate commit, right? If so then, IMO, it should be ok in case we want to revert it for some reason after the merge.
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 separated the commits intentionally as I anticipated a little bit this possibility
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 have it all working then I am fine with it, I didn't look at the current state before I commented.
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.
Because of the run task of Gradle, the methods of SourceSet
need to return interface types to be able to create proxies on top of it otherwise you get error of the next type. I think that I have no other choices but to convert PathsCollection
into an interface, Is that ok with you?
2021-05-18T11:24:09.3755726Z Exception in thread "main" java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
2021-05-18T11:24:09.3757359Z at io.quarkus.launcher.QuarkusLauncher.launch(QuarkusLauncher.java:58)
2021-05-18T11:24:09.3758732Z at io.quarkus.runtime.Quarkus.launchFromIDE(Quarkus.java:96)
2021-05-18T11:24:09.3759816Z at io.quarkus.runtime.Quarkus.run(Quarkus.java:83)
2021-05-18T11:24:09.3761347Z at io.quarkus.runtime.Quarkus.run(Quarkus.java:42)
2021-05-18T11:24:09.3762280Z at io.quarkus.runtime.Quarkus.run(Quarkus.java:119)
2021-05-18T11:24:09.3763157Z at org.acme.EntryPoint.main(EntryPoint.java:9)
2021-05-18T11:24:09.3764548Z Caused by: java.lang.reflect.InvocationTargetException
2021-05-18T11:24:09.3766148Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2021-05-18T11:24:09.3768290Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2021-05-18T11:24:09.3770650Z at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2021-05-18T11:24:09.3772555Z at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2021-05-18T11:24:09.3773843Z at io.quarkus.launcher.QuarkusLauncher.launch(QuarkusLauncher.java:56)
2021-05-18T11:24:09.3775191Z ... 5 more
2021-05-18T11:24:10.1218864Z Caused by: java.lang.RuntimeException: java.lang.IllegalArgumentException: io.quarkus.bootstrap.model.PathsCollection is not an interface
2021-05-18T11:24:10.1234536Z at io.quarkus.bootstrap.IDELauncherImpl.launch(IDELauncherImpl.java:85)
2021-05-18T11:24:10.1235855Z ... 10 more
2021-05-18T11:24:10.1237283Z Caused by: java.lang.IllegalArgumentException: io.quarkus.bootstrap.model.PathsCollection is not an interface
2021-05-18T11:24:10.1239583Z at java.base/java.lang.reflect.Proxy$ProxyBuilder.validateProxyInterfaces(Proxy.java:688)
2021-05-18T11:24:10.1241123Z at java.base/java.lang.reflect.Proxy$ProxyBuilder.<init>(Proxy.java:627)
2021-05-18T11:24:10.1242321Z at java.base/java.lang.reflect.Proxy$ProxyBuilder.<init>(Proxy.java:635)
2021-05-18T11:24:10.1243802Z at java.base/java.lang.reflect.Proxy.lambda$getProxyConstructor$0(Proxy.java:415)
2021-05-18T11:24:10.1249164Z at java.base/jdk.internal.loader.AbstractClassLoaderValue$Memoizer.get(AbstractClassLoaderValue.java:329)
2021-05-18T11:24:10.1252515Z at java.base/jdk.internal.loader.AbstractClassLoaderValue.computeIfAbsent(AbstractClassLoaderValue.java:205)
2021-05-18T11:24:10.1258098Z at java.base/java.lang.reflect.Proxy.getProxyConstructor(Proxy.java:413)
2021-05-18T11:24:10.1260384Z at java.base/java.lang.reflect.Proxy.newProxyInstance(Proxy.java:1006)
2021-05-18T11:24:10.1264309Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter.createView(ProtocolToModelAdapter.java:160)
2021-05-18T11:24:10.1267630Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter.convert(ProtocolToModelAdapter.java:276)
2021-05-18T11:24:10.1280517Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter.access$1500(ProtocolToModelAdapter.java:56)
2021-05-18T11:24:10.1283751Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter$AdaptingMethodInvoker.invoke(ProtocolToModelAdapter.java:477)
2021-05-18T11:24:10.1287361Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter$PropertyCachingMethodInvoker.invoke(ProtocolToModelAdapter.java:705)
2021-05-18T11:24:10.1290567Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter$SafeMethodInvoker.invoke(ProtocolToModelAdapter.java:742)
2021-05-18T11:24:10.1305810Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter$SupportedPropertyInvoker.invoke(ProtocolToModelAdapter.java:766)
2021-05-18T11:24:10.1309379Z at org.gradle.tooling.internal.adapter.ProtocolToModelAdapter$InvocationHandlerImpl.invoke(ProtocolToModelAdapter.java:432)
2021-05-18T11:24:10.1311418Z at com.sun.proxy.$Proxy17.getSourceDirectories(Unknown Source)
2021-05-18T11:24:10.1312963Z at io.quarkus.bootstrap.util.QuarkusModelHelper.convert(QuarkusModelHelper.java:147)
2021-05-18T11:24:10.1315315Z at io.quarkus.bootstrap.util.QuarkusModelHelper.serializeAppModel(QuarkusModelHelper.java:62)
2021-05-18T11:24:10.1317757Z at io.quarkus.bootstrap.util.QuarkusModelHelper.exportModel(QuarkusModelHelper.java:50)
2021-05-18T11:24:10.1353249Z at io.quarkus.bootstrap.utils.BuildToolHelper.enableGradleAppModelForDevMode(BuildToolHelper.java:120)
2021-05-18T11:24:10.1355698Z at io.quarkus.bootstrap.IDELauncherImpl.launch(IDELauncherImpl.java:41)
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.
Oh, right. Yes.
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 6ad1612
Full information is available in the Build summary check run. Test Failures⚙️ Devtools Tests - JDK 11 #📦 integration-tests/devtools✖ ✖ ✖ ✖ ✖ ✖ ⚙️ Devtools Tests - JDK 11 Windows #📦 integration-tests/devtools✖ ✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 11 #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 11 Windows #📦 devtools/cli✖ |
ab84c01
to
42c0337
Compare
If I move, e.g. |
Good to know, I will move the interface tomorrow |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a9bc97f
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✖ ✖ |
You mean moving PathsCollection to the gradle-resolver? It doesn't seems like a good idea. I'd prefer moving the QuarkusModel to the app-model, assuming it works, I'll check today. I'm puzzled what makes the gradle-resolver module special. It looks like it really prevents us having a common app model API between Maven and Gradle. |
Ok, moving all the interfaces and the impl classes to the |
@aloubyansky I can do it if you want in a separated commit within the context of this PR, WDYT? |
I just already have it moved in my branch already. I didn't touch the PathsCollection though. I just need to push it. |
@aloubyansky ok then ping me when it's merged |
@essobedo it's in |
a9bc97f
to
ca25d8d
Compare
@aloubyansky rebase done, let's see if it works now |
ca25d8d
to
ef54b02
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building ef54b02
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✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/resteasy-reactive-rest-client-standalone✖ |
ef54b02
to
f7094fd
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f7094fd
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✖ |
f7094fd
to
15145b7
Compare
@aloubyansky It should be fine now, the Gradle tests pass locally so I think that the PR is ready for review |
At last, the build is finally green 😅 |
Congrats! |
Set<String> sourceParents = new HashSet<>(); | ||
for (File srcDir : module.getSourceSourceSet().getSourceDirectories()) { | ||
sourceDirectories.add(srcDir.getPath()); | ||
final Set<Path> sourceParents = new LinkedHashSet<>(); |
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 not create PathsCollection directly here? Is it because it still allows duplicates?
Thanks @essobedo, great work! |
fixes #10298
Motivation
So far, the dev mode supports at most one resource folder and one test resource folder. In some particular use cases, it can helpful to have more than one resource folder. For example to refer common resources from a parent directory or in case you use an external yaml configuration file that you don't want to ship in the final artifact but you want to use for the dev mode.
Modifications
PathsCollection
instead of a String for the resource and test resource foldersSet
ofFile
or path withPathsCollection