[BUG] [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided file manager #3446
Conversation
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
| File file = locPath.toFile(); | ||
| IModule mod = ModuleFinder.scanForModule(this, file, null, true, null); | ||
| if (mod != null) { | ||
| return; |
There was a problem hiding this comment.
Several locations now assume that each ClasspathJsr199 is either JRT or a single module classpath location. See in particular new uses of the inherited field module. At a second look this is probably wrong, and each ClasspathJsr199 must be able to handle multiple modules.
Is this initialization the place that should actually create a Map<Location,IModule>? I'm a bit confused by the multi-level multiplicities: an Iterable of Sets of Locations where each Location has multiple Paths. At which of these levels can we assume a unique module? Is a LocationWrapper the right level? So if a module is in a jar, can we assume that it is represented by one LocationWrapper containing exactly one path?
| // do nothing | ||
| if (this.jrt != null) | ||
| return; // do nothing | ||
| this.module = mod; |
There was a problem hiding this comment.
If we need to handle multiple modules, then we probably need to let initialize() remember the module, since only there we know the corresponding LocationWrapper.
| public char[][] listPackages() { | ||
| Set<String> packageNames = new HashSet<>(); | ||
| try { | ||
| for (JavaFileObject fileObject : this.fileManager.list(this.location, "", fileTypes, true)) { //$NON-NLS-1$ |
There was a problem hiding this comment.
Listing all class files in all packages is surely a performance issue. Also passing "" to signal no filtering is a bit crude (needed an adjustment inside ModuleFinder, too). Unfortunately I found no better we to list all packages, which, however, the compiler needs particularly for auto modules.
Also, can we assume that 3rd-party file managers will handle "" in the same way?
| } | ||
| return result; | ||
| } catch (IOException e) { | ||
| // ?? |
There was a problem hiding this comment.
I don't know what's the best strategy to handle exceptions in this code area.
| Set<String> moduleNames = new HashSet<>(); | ||
| try { | ||
| for (Set<Location> locs : this.fileManager.listLocationsForModules(this.location)) { | ||
| for (Location loc : locs) { | ||
| String moduleName = this.fileManager.inferModuleName(loc); | ||
| if (moduleName != null) | ||
| moduleNames.add(moduleName); | ||
| } | ||
| } | ||
| return moduleNames; |
There was a problem hiding this comment.
If the implementation is good, it could perhaps benefit from caching.
But even the sequence of invocations looks funny: we list module locations, so those locations are already known to contain a module. Is it really necessary to then "infer" the module name? Wait, it seems we could use if (loc instanceof ModuleLocationWrapper wrapper) .. wrapper.getModuleName().
To do so we'd need to
make that class publicprovide agetModuleName()accessor
Next, I'm not even sure if it is safe to use our classes like LocationWrapper. Wouldn't that break if a custom file manager is used??
There was a problem hiding this comment.
Wait, it seems we could use ... ModuleLocationWrapper ...
Never mind, inferModuleName() is implemented in that exact way I was considering for the client side, so this part looks OK :)
Still caching the set of module names might be a good idea.
| @@ -269,6 +329,11 @@ public IModule getModule(char[] name) { | |||
| return super.getModule(name); | |||
| } | |||
|
|
|||
| @Override | |||
| public IModule getModule() { | |||
| return this.module; | |||
There was a problem hiding this comment.
Called by the compiler. If we handle multiple modules, how should the compiler ask for a specific module?
At this point I wonder if the ClasspathJsr199 with Location MODULE_PATH should actually be resolved to a set of classpaths, just like ClasspathContainers in the IDE?
There was a problem hiding this comment.
Yep, looks like it's not adequate for such scenarios. ClasspathJsr199 is more of a version of ClasspathLocation that knows a bit about Location but not doing a good job of morphing into a Location.
There was a problem hiding this comment.
Should I try to move the loops from ClasspathJsr199.initialize() into EclipseCompilerImpl.handleLocations() so that we can create individual ClasspathJsr199 per module?
| } | ||
| } catch (IllegalArgumentException iae) { // e.g., from ModuleFinder.scanForModule(Classpath, File, Parser, boolean, String) | ||
| // FIXME ignore for now |
There was a problem hiding this comment.
CompilerToolJava9Tests.testBug566749() expects a regular compile error, which now additionally surfaces as the IllegalArgumentException caught here.
| Iterable<Set<Location>> locationsForModules = this.fileManager.listLocationsForModules(this.location); | ||
| for (Set<Location> locs: locationsForModules) { | ||
| for (Location loc : locs) { | ||
| if (loc instanceof LocationWrapper wrapper) { |
There was a problem hiding this comment.
This use of LocationWrapper leaks an abstraction, it will not work with other file managers.
But then, how to find the contained paths??
There was a problem hiding this comment.
If it were a StandardJavaFileManager we could use getLocationAsPaths(Location)
|
In 477cab7 I changed the strategy towards using one ClasspathJsr199 per module. @jarthana from my p.o.v. this already looks much better. So perhaps you could take another look? I have some local changes attempting to support also MODULE_SOURCE_PATH, but it seems, ClasspathJsr199 is not suitable for handling sources. |
|
Test failed with the same bogus error as has been blocking #3388 for some time now. |
I would try to rebase the change on top of master as a new release-stream has started in the meanwhile. |
The change is based on I20241211-1800. Are you saying a new stream has started just within the last three days?? It looks like the stream change happened on 2024-11-23. |
file manager eclipse-jdt#958 ClasspathJsr199: + various drafty additions for handling modules ModuleFinder: + find automatic modules, too + allow invocation without a parser for binary-only scans EclipseFileManager: + allow scanning all packages (by specifying an empty package name) + establish symmetry between setLocation() and setLocationFromPaths() + catch disruptive IAE from ModuleFinder.scanForModule()
file manager + change to 1 ClasspathJsr199 per module + test with two module dependencies (one auto, one regular) Fixes eclipse-jdt#958
stephan-herrmann
force-pushed
the
issue954
branch
from
477cab7 to
c16af25
Compare
|
At the time of my comment there where three commits and at least one of them looks quite old, the other was a merge commit and then a "recent" one, beside that github showed merge conflicts. All these can probably lead to the build being confused about the actual baseline in use. Now you fixed that up, the build seem working fine (all checks are green). |
I have no clue where you saw this, but of course after you made me rebase the branch, I'm no longer able to prove that the branch indeed started from HEAD of master only 4 days ago.
I wouldn't call this strategy "fixing" but working around same intermittent bug. And of course none of this is of any help for #3388 |
Contributions are always welcome to fix such "bugs", but as JDT is not a standalone project several things influence the outcome (e.g. changes to aggregator parent pom, I-Build repositories and so on) of a PR that depends on such setups. |
This is not helpful. |
I showed you how to resolve the issue and it works, I'm not sure how to be more helpful... instead the solution is claimed a "workaround" for a bug. |
I'm hoping for a solution that is applicable also for BETA_JAVA24 without requiring that branches need to be tightly in sync, which would be an unacceptable requirement. Also I'm hoping for a solution where I'm able to understand the connection between the error and the remedy. |
The build infrastructure for (feature) branches is currently to ensure they can be merged into master at some point in time what requires them to be at least close to the target branch. Why this is unacceptable requirement is beyond my knowledge it more feels like a personal preference than a technical requirement, at least most contributors have adapted to the process to keep branches up-to date. Anyways there are some minimal requirements such a branch must fulfill e.g. it requires to include changes in version of bundles compared to the current integration build and it should not have any merge conflicts.
I hoped to explain it here its a bit hard to keep track of what is said where, but to summarize for this particular issue. Where it was failing beforeLets look at the last failing build logs here and we see it fails with: If we search up for the offending project we will see this (baseline-replace with latest ibuild): We see that actually there is no baseline version found, because its version was changed on master here 5 days ago (as of today) to version Now the next check kicks in (baseline-compare with latest release): it complains that you have build the exact same version as in the previous release but the content slightly differs what is classified as an error for platform builds as it usually means you have a missing version increment or something else is badly wrong. Please note that at this point there are no ignores! And of course this is not a bug... How it was fixedYou have rebased your branch on top of master and thus now it includes the version change as well, you could have merged master but we don't like merge commits here... another option would have been a squash merge (and later rebase) but this would pollute your PR with unrelated diffs / code changes maybe. Why it is working afterwardsLets look at the last successful build logs here and we see: So we see the baseline artifact from the last ibuild was found and your currently build jar was compared to the baseline and it was found to be semantically equal (and this is where we taking ignores into account) and therefore replaced by the jar from the last ibuild. Later on then we see so no error at all here, because both artifacts have different content (as the bundle was changed) but also different version that is higher than in the last release. |
ClasspathJsr199:
ModuleFinder:
EclipseFileManager:
Fixes #958