Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+62 −72
Open

Move createProblemMarker() to AbstractProjectConfigurator and clean up #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 51 additions & 31 deletions ...e.m2e.core/src/org/eclipse/m2e/core/project/configurator/AbstractProjectConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2008-2010 Sonatype, Inc.
* Copyright (c) 2008-2022 Sonatype, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
Expand All @@ -14,15 +14,19 @@
package org.eclipse.m2e.core.project.configurator;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Stream;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.eclipse.core.resources.IMarker;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IProjectDescription;
import org.eclipse.core.resources.IResource;
Expand All @@ -41,9 +45,13 @@
import org.eclipse.m2e.core.MavenPlugin;
import org.eclipse.m2e.core.embedder.IMaven;
import org.eclipse.m2e.core.embedder.IMavenConfiguration;
import org.eclipse.m2e.core.internal.IMavenConstants;
import org.eclipse.m2e.core.internal.Messages;
import org.eclipse.m2e.core.internal.lifecyclemapping.LifecycleMappingFactory;
import org.eclipse.m2e.core.internal.markers.IMavenMarkerManager;
import org.eclipse.m2e.core.internal.markers.MavenProblemInfo;
import org.eclipse.m2e.core.internal.markers.SourceLocation;
import org.eclipse.m2e.core.internal.markers.SourceLocationHelper;
import org.eclipse.m2e.core.lifecyclemapping.model.IPluginExecutionMetadata;
import org.eclipse.m2e.core.lifecyclemapping.model.PluginExecutionAction;
import org.eclipse.m2e.core.project.IMavenProjectChangedListener;
Expand Down Expand Up @@ -167,15 +175,43 @@ public static void addNature(IProject project, String natureId, int updateFlags,
throws CoreException {
if(!project.hasNature(natureId)) {
IProjectDescription description = project.getDescription();
String[] prevNatures = description.getNatureIds();
String[] newNatures = new String[prevNatures.length + 1];
System.arraycopy(prevNatures, 0, newNatures, 1, prevNatures.length);
newNatures[0] = natureId;
description.setNatureIds(newNatures);
var natures = Stream.concat(Stream.of(natureId), Arrays.stream(description.getNatureIds()));
description.setNatureIds(natures.toArray(String[]::new));
project.setDescription(description, updateFlags, monitor);
}
}

/**
* Creates a problem marker in the pom.xml file at the specified element of the given Plugin-execution with the passed
* message and severity.
* <p>
* If the attribute of the execution is specified in a parent pom.xml outside of the workspace the marker is created
* at the parent-element of the request's project.
* </p>
*
* @param execution the execution
* @param element the XML-element to mark
* @param request the request of the project being build
* @param problemSeverity the problems severity, one of {@link IMarker#SEVERITY_INFO SEVERITY_INFO},
* {@link IMarker#SEVERITY_WARNING SEVERITY_WARNING} or {@link IMarker#SEVERITY_ERROR SEVERITY_ERROR}
* @param problemMessage the message of the created problem marker
*/
protected void createProblemMarker(MojoExecution execution, String element, ProjectConfigurationRequest request,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the IMarker be returned here?
What if one wants to create the marker on the value of the element? Can't we give it a text position instead, and an helper to locate an element in a mojoExecution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the IMarker be returned here? What if one wants to create the marker on the value of the element?

In general IMavenMarkerManager could be changed to return the marker too.
Creating the marker at the value is not yet supported, but in general could be considered.

Can't we give it a text position instead, and an helper to locate an element in a mojoExecution?

Yes Christoph suggested that as well before.
And after some rethinking about it, it is probably the better way for a generalization because IIRC for markers of custom Ids m2e just creates them using the Eclipse Resources API. So there is no benefit in providing an extra API for that. But I have to check that again.
However I think for a simple case a convenient API method like this would be useful nevertheless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I think for a simple case a convenient API method like this would be useful nevertheless.

There is uncertainity with this API that needs to be covered before it can be claimed "API-quality" (ie we won't want to change it for a long time), and this comes by nature of using the "element" name:

  • How to place a marker on the value or a subset of the value? Or on some attribute (some mojo configuration may like it?
  • What happens is the element is repeated in the configuration?
  • What happens if there is no such element in the configuration?
  • ...

All those questions wouldn't be raised if we clearly separate locating the marker vs creating it. If it is clear that locating the marker is the important part, and not creating it (because Platform API is enough), then instead of API to create a marker, we should have an API to reoslve locations and use already existing satisfying APIs from Platform to create it.

int problemSeverity, String problemMessage) {
SourceLocation location = SourceLocationHelper.findLocation(execution.getPlugin(), element);

String[] gav = location.getResourceId().split(":");
IMavenProjectFacade facade = projectManager.getMavenProject(gav[0], gav[1], gav[2]);
if(facade == null) {
// attribute specifying project (probably parent) is not in the workspace.
// The following code returns the location of the project's parent-element.
location = SourceLocationHelper.findLocation(request.mavenProject(), new MojoExecutionKey(execution));
facade = request.mavenProjectFacade();
}
MavenProblemInfo problem = new MavenProblemInfo(problemMessage, problemSeverity, location);
markerManager.addErrorMarker(facade.getPom(), IMavenConstants.MARKER_CONFIGURATION_ID, problem);
}

/**
* @since 1.4
*/
Expand Down Expand Up @@ -216,21 +252,11 @@ public boolean equals(Object obj) {
if(this == obj) {
return true;
}
if(obj == null) {
return false;
}
if(getClass() != obj.getClass()) {
if(obj == null || getClass() != obj.getClass()) {
return false;
}
AbstractProjectConfigurator other = (AbstractProjectConfigurator) obj;
if(getId() == null) {
if(other.getId() != null) {
return false;
}
} else if(!getId().equals(other.getId())) {
return false;
}
return true;
return Objects.equals(getId(), other.getId());
}

/**
Expand All @@ -242,7 +268,7 @@ protected List<MojoExecution> getMojoExecutions(ProjectConfigurationRequest requ

Map<String, Set<MojoExecutionKey>> configuratorExecutions = getConfiguratorExecutions(projectFacade);

ArrayList<MojoExecution> executions = new ArrayList<>();
List<MojoExecution> executions = new ArrayList<>();

Set<MojoExecutionKey> executionKeys = configuratorExecutions.get(id);
if(executionKeys != null) {
Expand All @@ -259,25 +285,19 @@ protected List<MojoExecution> getMojoExecutions(ProjectConfigurationRequest requ
*/
public static Map<String, Set<MojoExecutionKey>> getConfiguratorExecutions(IMavenProjectFacade projectFacade) {
Map<String, Set<MojoExecutionKey>> configuratorExecutions = new HashMap<>();
Map<MojoExecutionKey, List<IPluginExecutionMetadata>> executionMapping = projectFacade.getMojoExecutionMapping();
for(Map.Entry<MojoExecutionKey, List<IPluginExecutionMetadata>> entry : executionMapping.entrySet()) {
List<IPluginExecutionMetadata> metadatas = entry.getValue();
projectFacade.getMojoExecutionMapping().forEach((key, metadatas) -> {
if(metadatas != null) {
for(IPluginExecutionMetadata metadata : metadatas) {
if(metadata.getAction() == PluginExecutionAction.configurator) {
String configuratorId = LifecycleMappingFactory.getProjectConfiguratorId(metadata);
if(configuratorId != null) {
Set<MojoExecutionKey> executions = configuratorExecutions.get(configuratorId);
if(executions == null) {
executions = new LinkedHashSet<>();
configuratorExecutions.put(configuratorId, executions);
}
executions.add(entry.getKey());
String id = LifecycleMappingFactory.getProjectConfiguratorId(metadata);
if(id != null) {
Set<MojoExecutionKey> executions = configuratorExecutions.computeIfAbsent(id, i -> new LinkedHashSet<>());
executions.add(key);
}
}
}
}
}
});
return configuratorExecutions;
}

Expand Down
31 changes: 3 additions & 28 deletions ...m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,9 @@
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.m2e.core.MavenPlugin;
import org.eclipse.m2e.core.embedder.IMaven;
import org.eclipse.m2e.core.internal.IMavenConstants;
import org.eclipse.m2e.core.internal.markers.IMavenMarkerManager;
import org.eclipse.m2e.core.internal.markers.MavenProblemInfo;
import org.eclipse.m2e.core.internal.markers.SourceLocation;
import org.eclipse.m2e.core.internal.markers.SourceLocationHelper;
import org.eclipse.m2e.core.lifecyclemapping.model.IPluginExecutionMetadata;
import org.eclipse.m2e.core.project.IMavenProjectFacade;
import org.eclipse.m2e.core.project.IMavenProjectRegistry;
import org.eclipse.m2e.core.project.configurator.AbstractBuildParticipant;
import org.eclipse.m2e.core.project.configurator.AbstractProjectConfigurator;
import org.eclipse.m2e.core.project.configurator.ILifecycleMappingConfiguration;
Expand Down Expand Up @@ -70,7 +65,8 @@ public void configure(ProjectConfigurationRequest request, IProgressMonitor moni
Boolean supportIncremental = maven.getMojoParameterValue(request.mavenProject(), execution,
FELIX_PARAM_SUPPORTINCREMENTALBUILD, Boolean.class, monitor);
if (supportIncremental == null || !supportIncremental.booleanValue()) {
createWarningMarker(request, execution, SourceLocationHelper.CONFIGURATION,
createProblemMarker(execution, SourceLocationHelper.CONFIGURATION, request,
IMarker.SEVERITY_WARNING,
"Incremental updates are currently disabled, set supportIncrementalBuild=true to support automatic manifest updates for this project.");
}

Expand All @@ -82,7 +78,7 @@ public void configure(ProjectConfigurationRequest request, IProgressMonitor moni
}
if (!hasManifestExecution && !executions.isEmpty()) {
MojoExecution execution = executions.get(0);
createWarningMarker(request, execution, "executions",
createProblemMarker(execution, "executions", request, IMarker.SEVERITY_WARNING,
"There is currently no execution that generates a manifest, consider adding an execution for one of the following goal: "
+ (isFelix(execution.getPlugin()) ? FELIX_MANIFEST_GOAL : BND_MANIFEST_GOALS) + ".");
}
Expand All @@ -92,27 +88,6 @@ public void configure(ProjectConfigurationRequest request, IProgressMonitor moni
PDEProjectHelper.addPDENature(facade.getProject(), metainfPath, monitor);
}

private void createWarningMarker(ProjectConfigurationRequest request, MojoExecution execution, String attribute,
String message) {
createWarningMarker(projectManager, markerManager, request, execution, attribute, message);
}

static void createWarningMarker(IMavenProjectRegistry projectManager, IMavenMarkerManager markerManager,
ProjectConfigurationRequest request, MojoExecution execution, String attribute, String message) {
SourceLocation location = SourceLocationHelper.findLocation(execution.getPlugin(), attribute);

String[] gav = location.getResourceId().split(":");
IMavenProjectFacade facade = projectManager.getMavenProject(gav[0], gav[1], gav[2]);
if (facade == null) {
// attribute specifying project (probably parent) is not in the workspace.
// The following code returns the location of the project's parent-element.
location = SourceLocationHelper.findLocation(request.mavenProject(), new MojoExecutionKey(execution));
facade = request.mavenProjectFacade();
}
MavenProblemInfo problem = new MavenProblemInfo(message, IMarker.SEVERITY_WARNING, location);
markerManager.addErrorMarker(facade.getPom(), IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problem);
}

private boolean isFelixManifestGoal(MojoExecution execution) {
return FELIX_MANIFEST_GOAL.equals(execution.getGoal());
}
Expand Down
21 changes: 8 additions & 13 deletions ...ipse.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/TychoPackagingsConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Konrad Windszus
* Konrad Windszus - initial API and implementation
*******************************************************************************/
package org.eclipse.m2e.pde.connector;

Expand All @@ -18,6 +18,7 @@
import org.apache.maven.plugin.MojoExecution;
import org.apache.maven.project.MavenProject;
import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.eclipse.core.resources.IMarker;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.ProjectScope;
import org.eclipse.core.runtime.CoreException;
Expand Down Expand Up @@ -61,10 +62,9 @@ private void applyDsConfiguration(ProjectConfigurationRequest request, IProgress
return;
}
if (mojoExecutions.size() > 1) {
String message = String.format(
"Found more than one execution for plugin %s:%s and goal %s, only consider configuration of this one",
TYCHO_GROUP_ID, TYCHO_DS_PLUGIN_ARTIFACT_ID, GOAL_DECLARATIVE_SERVICES);
createWarningMarker(request, mojoExecutions.get(0), "executions", message);
createProblemMarker(mojoExecutions.get(0), "executions", request, IMarker.SEVERITY_WARNING,
"Found more than one execution for plugin " + TYCHO_GROUP_ID + ":" + TYCHO_DS_PLUGIN_ARTIFACT_ID
+ " and goal " + GOAL_DECLARATIVE_SERVICES + ", only consider configuration of this one");
}
// first mojo execution is relevant
Xpp3Dom dom = mojoExecutions.get(0).getConfiguration();
Expand All @@ -85,8 +85,9 @@ private void applyDsConfiguration(ProjectConfigurationRequest request, IProgress
if (version != null) {
prefs.put(org.eclipse.pde.ds.internal.annotations.Activator.PREF_SPEC_VERSION, version.name());
} else {
String message = "Unsupported DS spec version " + versionValue + " found, using default instead";
createWarningMarker(request, mojoExecutions.get(0), SourceLocationHelper.CONFIGURATION, message);
createProblemMarker(mojoExecutions.get(0), SourceLocationHelper.CONFIGURATION, request,
IMarker.SEVERITY_WARNING,
"Unsupported DS spec version " + versionValue + " found, using default instead");
}
}
Xpp3Dom path = dom.getChild("path");
Expand Down Expand Up @@ -124,10 +125,4 @@ private List<MojoExecution> getTychoDsPluginMojoExecutions(ProjectConfigurationR
return request.mavenProjectFacade().getMojoExecutions(TYCHO_GROUP_ID, TYCHO_DS_PLUGIN_ARTIFACT_ID, monitor,
GOAL_DECLARATIVE_SERVICES);
}

private void createWarningMarker(ProjectConfigurationRequest request, MojoExecution execution, String attribute,
String message) {
PDEMavenBundlePluginConfigurator.createWarningMarker(projectManager, markerManager, request, execution,
attribute, message);
}
}