From ab1385925a063ec981094a3a4ef6187613af58fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20L=C3=A4ubrich?= Date: Sat, 25 Jan 2025 08:29:22 +0100 Subject: [PATCH] Let QuickFix run in UI, save editor, rebuild on error and wait for jobs Fixing one warning might make the resulting markers either invalid or no longer apply. Also quickfixes are often written to be run in the UI thread and might schedule jobs during execution. This now rebuilds the project when one marker was fixed and run them in the UI while waiting for any jobs scheduled in the meantime. --- .../eclipse/tycho/cleancode/CleanUpMojo.java | 4 +- .../org/eclipse/tycho/cleancode/QuickFix.java | 130 +++++++++++++----- .../eclipse/tycho/cleancode/QuickFixMojo.java | 8 +- .../tycho/cleancode/QuickFixResult.java | 27 ++-- .../eclipse/tycho/core/MarkdownBuilder.java | 19 +++ .../eclipsebuild/AbstractEclipseBuild.java | 72 +++++++++- .../AbstractEclipseBuildMojo.java | 4 +- .../tycho/eclipsebuild/SetTargetPlatform.java | 19 ++- 8 files changed, 227 insertions(+), 56 deletions(-) diff --git a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/CleanUpMojo.java b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/CleanUpMojo.java index 03601b0bdd..21e390e323 100644 --- a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/CleanUpMojo.java +++ b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/CleanUpMojo.java @@ -62,10 +62,12 @@ protected void handleResult(CleanupResult result) return; } MarkdownBuilder builder = new MarkdownBuilder(reportFileName); - builder.add("The following cleanups where applied:"); + builder.h3("The following cleanups where applied:"); result.cleanups().forEach(cleanup -> { builder.addListItem(cleanup); }); + builder.newLine(); + builder.newLine(); builder.write(); } diff --git a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFix.java b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFix.java index a76f23c22a..0bde09e3f7 100644 --- a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFix.java +++ b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFix.java @@ -15,16 +15,23 @@ import java.nio.file.Path; import java.util.Arrays; import java.util.Comparator; -import java.util.concurrent.atomic.AtomicReference; +import java.util.LinkedHashSet; import org.eclipse.core.resources.IMarker; import org.eclipse.core.resources.IProject; import org.eclipse.core.resources.IResource; +import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.ICoreRunnable; +import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.Status; +import org.eclipse.core.runtime.jobs.Job; import org.eclipse.tycho.eclipsebuild.AbstractEclipseBuild; import org.eclipse.ui.IMarkerResolution; import org.eclipse.ui.IMarkerResolution2; import org.eclipse.ui.IMarkerResolutionRelevance; +import org.eclipse.ui.PlatformUI; import org.eclipse.ui.ide.IDE; +import org.eclipse.ui.progress.UIJob; /** * Applies the QuickFix with the highest relevance to a warning, because @@ -40,44 +47,81 @@ public class QuickFix extends AbstractEclipseBuild { @Override protected QuickFixResult createResult(IProject project) throws Exception { QuickFixResult result = new QuickFixResult(); - IMarker[] markers = project.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE); - result.setNumberOfMarker(markers.length); + while (fixOneMarker(project, result)) { + runInUI("Save Editors", m -> { + if (PlatformUI.isWorkbenchRunning()) { + PlatformUI.getWorkbench().saveAllEditors(false); + } + }); + debug("### Perform build to update markers ###"); + buildProject(project); + } + return result; + } + + private boolean fixOneMarker(IProject project, QuickFixResult result) throws CoreException { + + debug("### Check for marker with resolutions..."); + IMarker[] markers = getCurrentMarker(project, true); for (IMarker marker : markers) { - debug("Marker: " + marker); - try { - IMarkerResolution[] resolutions = IDE.getMarkerHelpRegistry().getResolutions(marker); - debug("Marker has " + resolutions.length + " resolutions"); - IMarkerResolution resolution = Arrays.stream(resolutions) - .max(Comparator.comparingInt(r -> getRelevance(r))).orElse(null); - if (resolution != null) { - debug("Apply best resolution to marker: " + getInfo(resolution)); - // must use an own thread to make sure it is not called as a job - AtomicReference error = new AtomicReference(); - Thread thread = new Thread(new Runnable() { - - @Override - public void run() { - try { + if (result.tryFix(marker)) { + debug("Check Marker: " + getInfo(marker)); + try { + IMarkerResolution[] resolutions = IDE.getMarkerHelpRegistry().getResolutions(marker); + debug("\tMarker has " + resolutions.length + " resolutions"); + IMarkerResolution resolution = Arrays.stream(resolutions) + .max(Comparator.comparingInt(r -> getRelevance(r))).orElse(null); + if (resolution != null) { + for (IMarkerResolution r : resolutions) { + debug(String.format("\t\t- (%d): %s", getRelevance(r), getInfo(r, false))); + } + LinkedHashSet jobs = new LinkedHashSet<>(); + IStatus status = runInUI("fix marker " + getInfo(marker), m -> { + AbstractEclipseBuild.recordJobs(jobs, m, nil -> { + debug("\tApply best resolution to marker: " + getInfo(resolution, true)); resolution.run(marker); - } catch (Throwable t) { - error.set(t); - } + }); + }); + for (Job job : jobs) { + debug("Wait for Job '" + job.getName() + "' scheduled during marker resolution..."); + job.join(); + } + if (status.isOK()) { + String fix = buildFixMessage(marker); + debug("\t" + fix); + result.addFix(fix); + return true; + } else { + debug("\tMarker could not be applied!", status.getException()); } - }); - thread.start(); - thread.join(); - Throwable t = error.get(); - if (t == null) { - result.addFix(buildFixMessage(marker)); - } else { - debug("Marker could not be applied!", t); } + } catch (Throwable t) { + debug("\tMarker resolutions could not be computed!", t); } - } catch (Throwable t) { - debug("Marker resolutions could not be computed!", t); } } - return result; + return false; + } + + private String getInfo(IMarker marker) { + return marker.getAttribute(IMarker.MESSAGE, "") + " @ " + marker.getResource() + ":" + + marker.getAttribute(IMarker.LINE_NUMBER, -1); + } + + private IMarker[] getCurrentMarker(IProject project, boolean rebuildOnError) throws CoreException { + IMarker[] markers = project.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE); + for (IMarker marker : markers) { + if (marker.getAttribute(IMarker.SEVERITY, -1) == IMarker.SEVERITY_ERROR) { + if (rebuildOnError) { + debug("Found error marker, try rebuild ..."); + buildProject(project); + return getCurrentMarker(project, false); + } + debug("Found error, can't apply fixes: " + getInfo(marker)); + return new IMarker[0]; + } + } + return markers; } private String buildFixMessage(IMarker marker) { @@ -95,8 +139,8 @@ private String buildFixMessage(IMarker marker) { return sb.toString(); } - private String getInfo(IMarkerResolution resolution) { - if (resolution instanceof IMarkerResolution2 res2) { + private String getInfo(IMarkerResolution resolution, boolean withDescription) { + if (withDescription && resolution instanceof IMarkerResolution2 res2) { return resolution.getClass().getName() + ": " + getLabel(resolution) + " // " + getDescription(res2); } else { return resolution.getClass().getName() + ": " + getLabel(resolution); @@ -110,7 +154,7 @@ private int getRelevance(IMarkerResolution resolution) { } } catch (RuntimeException e) { } - return -1; + return 0; } private String getDescription(IMarkerResolution2 markerResolution) { @@ -129,4 +173,20 @@ private String getLabel(IMarkerResolution resolution) { } } + private static IStatus runInUI(String action, ICoreRunnable runnable) throws InterruptedException { + UIJob job = UIJob.create(action, m -> { + try { + runnable.run(m); + } catch (CoreException e) { + return e.getStatus(); + } catch (Throwable e) { + return Status.error("Run failed", e); + } + return Status.OK_STATUS; + }); + job.schedule(); + job.join(); + return job.getResult(); + } + } diff --git a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixMojo.java b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixMojo.java index fce4b670b4..bdb96a57b9 100644 --- a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixMojo.java +++ b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixMojo.java @@ -52,9 +52,15 @@ protected void handleResult(QuickFixResult result) throws MojoFailureException { } MarkdownBuilder builder = new MarkdownBuilder(reportFileName); List fixes = result.fixes().toList(); - builder.add("The following " + (fixes.size() > 0 ? "warnings" : "warning") + " has been resolved:"); + builder.h3("The following " + (fixes.size() > 0 ? "warnings" : "warning") + " has been resolved:"); + builder.newLine(); fixes.forEach(fix -> builder.addListItem(fix)); + builder.newLine(); + builder.newLine(); builder.write(); + for (String fix : fixes) { + getLog().info("Fixed: " + fix); + } } @Override diff --git a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixResult.java b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixResult.java index 4e59a0851c..732438ae9c 100644 --- a/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixResult.java +++ b/tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixResult.java @@ -13,13 +13,18 @@ package org.eclipse.tycho.cleancode; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Stream; +import org.eclipse.core.resources.IMarker; +import org.eclipse.core.runtime.CoreException; import org.eclipse.tycho.eclipsebuild.EclipseBuildResult; public class QuickFixResult extends EclipseBuildResult { + private Set tried = new HashSet(); private List fixed = new ArrayList<>(); private int markers; @@ -31,16 +36,22 @@ public void addFix(String fix) { fixed.add(fix); } - public void setNumberOfMarker(int markers) { - this.markers = markers; - } - - public int getMarkers() { - return markers; - } - public boolean isEmpty() { return fixed.isEmpty(); } + public boolean tryFix(IMarker marker) { + String msg = marker.getAttribute(IMarker.MESSAGE, ""); + String resource = marker.getResource().toString(); + int line = marker.getAttribute(IMarker.LINE_NUMBER, -1); + String type; + try { + type = marker.getType(); + } catch (CoreException e) { + type = ""; + } + String key = type + " " + resource + ":" + line + " " + msg; + return tried.add(key); + } + } diff --git a/tycho-core/src/main/java/org/eclipse/tycho/core/MarkdownBuilder.java b/tycho-core/src/main/java/org/eclipse/tycho/core/MarkdownBuilder.java index 1d60a70b02..9391652a7d 100644 --- a/tycho-core/src/main/java/org/eclipse/tycho/core/MarkdownBuilder.java +++ b/tycho-core/src/main/java/org/eclipse/tycho/core/MarkdownBuilder.java @@ -57,4 +57,23 @@ public void write() throws MojoFailureException { } } + public void newLine() { + lines.add(""); + } + + public void h1(String string) { + lines.add("# " + string); + lines.add(""); + } + + public void h2(String string) { + lines.add("## " + string); + lines.add(""); + } + + public void h3(String string) { + lines.add("### " + string); + lines.add(""); + } + } diff --git a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuild.java b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuild.java index 5e67dbec89..09c53b4fc4 100644 --- a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuild.java +++ b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuild.java @@ -5,6 +5,8 @@ import java.io.Serializable; import java.io.StringWriter; import java.nio.file.Path; +import java.util.LinkedHashSet; +import java.util.Set; import java.util.concurrent.Callable; import org.eclipse.core.resources.IMarker; @@ -16,9 +18,12 @@ import org.eclipse.core.resources.IncrementalProjectBuilder; import org.eclipse.core.resources.ResourcesPlugin; import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.ICoreRunnable; import org.eclipse.core.runtime.IPath; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.Platform; +import org.eclipse.core.runtime.jobs.IJobChangeEvent; +import org.eclipse.core.runtime.jobs.IJobChangeListener; import org.eclipse.core.runtime.jobs.Job; /** @@ -54,10 +59,8 @@ public final Result call() throws Exception { } protected void buildProject(IProject project) throws CoreException { - project.build(IncrementalProjectBuilder.FULL_BUILD, this); - while (!Job.getJobManager().isIdle()) { - Thread.yield(); - } + debug("Building..."); + executeWithJobs(this, m -> project.build(IncrementalProjectBuilder.FULL_BUILD, m)); } protected abstract Result createResult(IProject project) throws Exception; @@ -69,6 +72,63 @@ static void disableAutoBuild() throws CoreException { workspace.setDescription(desc); } + public static void executeWithJobs(IProgressMonitor monitor, ICoreRunnable runnable) + throws CoreException { + Set scheduledJobs = recordJobs(new LinkedHashSet<>(), monitor, runnable); + for (Job job : scheduledJobs) { + if (monitor != null) { + monitor.subTask("Wait for job " + job.getName() + " to finish"); + } + try { + job.join(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + } + } + + public static Set recordJobs(Set scheduledJobs, IProgressMonitor monitor, ICoreRunnable runnable) + throws CoreException { + IJobChangeListener listener = new IJobChangeListener() { + + @Override + public void sleeping(IJobChangeEvent event) { + + } + + @Override + public void scheduled(IJobChangeEvent event) { + scheduledJobs.add(event.getJob()); + } + + @Override + public void running(IJobChangeEvent event) { + + } + + @Override + public void done(IJobChangeEvent event) { + scheduledJobs.remove(event.getJob()); + } + + @Override + public void awake(IJobChangeEvent event) { + + } + + @Override + public void aboutToRun(IJobChangeEvent event) { + + } + }; + Job.getJobManager().addJobChangeListener(listener); + IProgressMonitor safe = IProgressMonitor.nullSafe(monitor); + runnable.run(safe); + Job.getJobManager().removeJobChangeListener(listener); + return scheduledJobs; + } + private void deleteAllProjects() throws CoreException { for (IProject project : ResourcesPlugin.getWorkspace().getRoot().getProjects()) { project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT | IResource.FORCE, this); @@ -94,6 +154,10 @@ protected void debug(String string) { } protected void debug(String string, Throwable t) { + if (t == null) { + debug(string); + return; + } if (debug) { StringWriter writer = new StringWriter(); t.printStackTrace(new PrintWriter(writer)); diff --git a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuildMojo.java b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuildMojo.java index ff5b3492f3..f0a48a277c 100644 --- a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuildMojo.java +++ b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuildMojo.java @@ -81,7 +81,7 @@ public abstract class AbstractEclipseBuildMojo arguments; String applicationName = this.application; - boolean useApplication = applicationName != null; + boolean useApplication = applicationName != null && !applicationName.isBlank(); if (useApplication) { arguments = List.of(EclipseApplication.ARG_APPLICATION, applicationName); } else { diff --git a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/SetTargetPlatform.java b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/SetTargetPlatform.java index 816956a41d..494266e176 100644 --- a/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/SetTargetPlatform.java +++ b/tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/SetTargetPlatform.java @@ -30,7 +30,6 @@ import org.eclipse.pde.core.target.ITargetPlatformService; import org.eclipse.pde.core.target.LoadTargetDefinitionJob; import org.eclipse.pde.core.target.TargetBundle; -import org.eclipse.pde.internal.core.PluginModelManager; import org.eclipse.pde.internal.core.target.TargetPlatformService; public class SetTargetPlatform implements Callable, Serializable { @@ -63,10 +62,20 @@ public Serializable call() throws Exception { .toArray(TargetBundle[]::new); target.setTargetLocations(new ITargetLocation[] { new BundleListTargetLocation(bundles) }); service.saveTargetDefinition(target); - Job job = new LoadTargetDefinitionJob(target); - job.schedule(); - job.join(); - Job.getJobManager().join(PluginModelManager.class, new NullProgressMonitor()); + AbstractEclipseBuild.executeWithJobs(new NullProgressMonitor() { + + @Override + public void subTask(String name) { + debug(name); + }; + }, m -> { + Job job = new LoadTargetDefinitionJob(target); + job.schedule(); + try { + job.join(); + } catch (InterruptedException e) { + } + }); return null; }