-
Notifications
You must be signed in to change notification settings - Fork 399
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
Cascade project configuration changes to child projects #806
Conversation
} | ||
} | ||
|
||
public IProject[] collectProjects(IProject project, IProgressMonitor monitor) { |
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.
not sure about this approach. that'll trigger a full build of the project and its children IIUC.
I'd go with:
1- load project facade
2- check if packaging is pom, else bail
3- collect all modules from facade.mavenproject (does it include those from active profiles?)
4- for each module: compute path to module, see if matches an open IProject
5- add all found Maven projects. If packaging pom is found, see 1
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.
The caveat is that'd would not cover the case where the project is not a module of its parent 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.
I have used DefaultMaven.collectProjects(List, List, MavenExecutionRequest). I think it won't trigger a full build. See DefaultMaven.doExecute(MavenExecutionRequest).
I have tested the spring-boot project. MavenBuildSupport.collectProjects(IProject, IProgressMonitor) takes about 15 seconds, a full build more than three minutes.
I have also checked the algorithm you proposed.
public void collectProjects(Collection<IProject> projects, IProject project, IProgressMonitor monitor) {
if (!ProjectUtils.isMavenProject(project)) {
return;
}
projects.add(project);
IMavenProjectFacade projectFacade = registry.create(project, monitor);
if ("pom".equals(projectFacade.getPackaging())) {
List<String> modules = projectFacade.getMavenProjectModules();
for (String module : modules) {
IPath pomPath = ResourcesPlugin.getWorkspace().getRoot().getFullPath().append(module).append("pom.xml");
IFile pom = ResourcesPlugin.getWorkspace().getRoot().getFile(pomPath);
IProject p = pom.getProject();
if (p.isOpen()) {
collectProjects(projects, p, monitor);
}
}
}
}
Both of the methods return 173 projects.
Which method do you advice?
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.
15sec is for which implem?
Also your pomPath seems dubious. It will search for modules from the root of the workspace, not from the root of the project
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.
15sec is for which implem?
MavenBuildSupport.collectProjects(IProject, IProgressMonitor)
Also your pomPath seems dubious. It will search for modules from the root of the workspace, not from the root of the project
I have checked that.
("/spring-boot-build/spring-boot-project/pom.xml").getProject() returns P/spring-boot-build
("/spring-boot-project/pom.xml").getProject() returns P/spring-boot-project
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.
ok so what time for collectProjects(Collection projects, IProject project, IProgressMonitor monitor)?
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.
1-2 seconds
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.
2s vs 15s, there's no debate
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 your commit around (another branch) just in case and update the PR to use the faster version
@fbricon I have updated the PR. The previous version is in https:/snjeza/eclipse.jdt.ls/tree/issue-638first |
} | ||
|
||
public void collectProjects(Collection<IProject> projects, IProject project, IProgressMonitor monitor) { | ||
if (!ProjectUtils.isMavenProject(project)) { |
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.
add check if isOpen here
List<String> modules = projectFacade.getMavenProjectModules(); | ||
for (String module : modules) { | ||
IPath pomPath = ResourcesPlugin.getWorkspace().getRoot().getFullPath().append(module).append("pom.xml"); | ||
IFile pom = ResourcesPlugin.getWorkspace().getRoot().getFile(pomPath); |
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.
check if file exists
@@ -82,6 +85,14 @@ public void testCompileWithEclipseTychoJdt() throws Exception { | |||
testNonStandardCompilerId("compile-with-tycho-jdt"); | |||
} | |||
|
|||
@Test | |||
public void testMultipleProjects() throws Exception { | |||
IProject project = importMavenProject("multimodule"); |
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.
add another level of hierarchy, i.e. have a pom module with its on children
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.
also check for modules added through active profiles
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.
@fbricon I have updated the PR.
Signed-off-by: Snjezana Peco <[email protected]>
It sorta works but if you change the maven-compiler-plugin version or configuration in a parent pom for instance, and all children get the |
That's a bug on its own, I'll open a separate ticket |
Opened #814 |
Fixes redhat-developer/vscode-java#638
Signed-off-by: Snjezana Peco [email protected]