Update project if needed #547

bdshadow · 2018-02-10T19:33:46Z

although https://github.com/eclipse/eclipse.jdt.ls/pull/545/files was already merged, while i was working on this solution, still want to offer checking if pom.xml was changed not by comparing time, but by comparing md5 digests. In my opinion, it's more reliable, than checking the modification time of the whole workspace.

This PR is far not ideal, and will be updated, in case the idea is accepted.

fbricon · 2018-02-12T14:19:44Z

Although I agree using a hash-based comparison is more robust than the current timestamp check, the current PR has 2 major flaws:

hashes are not serialized, so subsequent server(JVM) starts will ignore previous hash check, so we lose the benefits of the current timestamp-based solution
more importantly, explicit/manual update command is ignored if the pom is deemed unchanged, which is wrong, if you changed a parent pom for instance and want to update one of its modules.

fbricon · 2018-02-12T14:22:02Z

Well this PR proves something's wrong with #545 tests, since they should have failed with those changes

bdshadow · 2018-02-12T15:28:35Z

@fbricon could you, please, take a look at the changes. I tried to fix your 2nd remark

bdshadow · 2018-02-15T14:39:10Z

@fbricon the build is fine for me locally. No test fails. Could you, please, restart the build in Jenkins and look at this pr again?

fbricon · 2018-02-15T15:59:19Z

test this please

fbricon · 2018-02-15T16:03:18Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

@@ -26,23 +35,47 @@
 */
 public class MavenBuildSupport implements IBuildSupport {

+	private static Map<Path, String> pomDigests = new ConcurrentHashMap<>();


We definitely need to serialize that map to disk, either after it changes (safer) or on workspace shutdown. Else subsequent server(JVM) starts will ignore previous hash check so it'll be a regression in behavior

@fbricon ok, working on it. Is there a special place where eclipse.jdt.ls serializes objects to? Or maybe you can advise the path

fbricon · 2018-02-26T19:16:58Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/IBuildSupport.java

+	 *            - a project to update
+	 * @param force
+	 *            - defines if the <code>project</code> must be updated despite of
+	 *            no changes of pom.xml are made


no changes in the build descriptor are made

fbricon · 2018-03-26T21:12:19Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

@@ -220,7 +220,8 @@ public void fileChanged(String uriString, CHANGE_TYPE changeType) {
 				FeatureStatus status = preferenceManager.getPreferences().getUpdateBuildConfigurationStatus();
 				switch (status) {
 					case automatic:
-						updateProject(resource.getProject());
+						// do not force the build, because it's not started by user and should be done only if pom.xml has changed


s/pom.xml/build file/

fbricon · 2018-03-26T21:14:18Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

@@ -315,7 +316,7 @@ public IProject createJavaProject(IProject project, IProgressMonitor monitor) th
 		return project;
 	}

-	public Job updateProject(IProject project) {


we might as well keep the update(IProject project) flavour, to avoid the need to change to the more verbose update(project, false) version all over the place. Needs javadoc explaining the 2 signatures

The method is only called twice from production code, so I think keeping it as is makes sense.

fbricon · 2018-03-26T21:19:16Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenProjectImporter.java

-					configurationManager.updateProjectConfiguration(request, monitor);
+					// maven update should not be forced here, because it's a new project, just imported into workspace and
+					// will be run automatically, because pom.xml-s have just appeared
+					new MavenBuildSupport(configurationManager).update(project, monitor);


move MavenBuildSupport instantiation out of the loop

Why not do projectsManage.getBuildSupport(project).update(project, monitor)? The assumption we have is that MavenBuildSupport is stateless, but I don't really like that.

fbricon · 2018-03-26T21:42:26Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

+			String newDigestStr = Arrays.toString(digest);
+			synchronized (pomDigests) {
+				String prevDigest = pomDigests.put(path.toString(), newDigestStr);
+				if (prevDigest == null || !prevDigest.equals(newDigestStr)) {


if (!newDigestStr.equals(prevDigest))

tsmaeder · 2018-03-26T15:03:29Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

+	static {
+		File workspaceFile = ResourcesPlugin.getWorkspace().getRoot().getRawLocation().makeAbsolute().toFile();
+		SERIALIZATION_FILE = new File(workspaceFile, POM_SERLIZATION_FILE_NAME);
+		if (SERIALIZATION_FILE.exists()) {


tsmaeder · 2018-03-26T15:05:21Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

+			synchronized (pomDigests) {
+				String prevDigest = pomDigests.put(path.toString(), newDigestStr);
+				if (prevDigest == null || !prevDigest.equals(newDigestStr)) {
+					serializePomDigests();


Should we serialize on every change?

tsmaeder · 2018-03-27T07:49:01Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenProjectImporter.java

-					configurationManager.updateProjectConfiguration(request, monitor);
+					// maven update should not be forced here, because it's a new project, just imported into workspace and
+					// will be run automatically, because pom.xml-s have just appeared
+					new MavenBuildSupport(configurationManager).update(project, monitor);


Why not do projectsManage.getBuildSupport(project).update(project, monitor)? The assumption we have is that MavenBuildSupport is stateless, but I don't really like that.

tsmaeder · 2018-03-27T07:53:41Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

@@ -26,23 +41,87 @@
 */
 public class MavenBuildSupport implements IBuildSupport {

+	private static final String POM_SERLIZATION_FILE_NAME = ".pom-digests";


I really don't like the fact that we're using a static map here. I guess it comes from the fact that MavenBuildSupport is used as a stateless object. I'd rather change that and keep the map in the instance.
Also, we should use the workspace state location available to eclipse plugins instead of using the workspace root.

fbricon · 2018-03-27T13:24:52Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaLanguageServerPlugin.java

 import org.osgi.util.tracker.ServiceTracker;

-public class JavaLanguageServerPlugin implements BundleActivator {
+public class JavaLanguageServerPlugin extends Plugin {


getStateLocation() is on plugin

fbricon · 2018-03-27T13:29:19Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

@@ -102,6 +104,7 @@ protected IStatus run(IProgressMonitor monitor) {

 	public ProjectsManager(PreferenceManager preferenceManager) {
 		this.preferenceManager = preferenceManager;
+		this.mavenBuildSupport = new MavenBuildSupport(MavenPlugin.getProjectConfigurationManager());


-1. I'd like to decouple ProjectsManager from underlying build supports eventually. I expect buildSupports() implementation to load IBuildSupport instances from extension points, and don't want to make any assumption about the statefulness of each IBuildSupport.

If you're creating a new instance every time you need one, you ARE making an assumption, namely that build support is stateless. Keeping the same instance allows the implementers to be stateless or not.
IMNSHO (;-)) this is much better than keeping the digests in a static map.

fbricon · 2018-03-27T13:32:00Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

 		MavenUpdateRequest request = new MavenUpdateRequest(project, MavenPlugin.getMavenConfiguration().isOffline(), true);
 		configurationManager.updateProjectConfiguration(request, monitor);
 	}

+	private boolean needsMavenUpdate(IProject project) {


the more I think of it the less I like this implementation. This method should simply check if a project needs an update. Updating the digests map should be done once the update is effectively triggered

But the two are really equivalent here, no? If the the digest is changed and an update needed, the project WILL be updated. So while not pretty, I think the logic is correct.

it works for now. Until needsMavenUpdate is called from somewhere else (even in the same class since it's private) and no update is performed, then things will go wrong.
calling needsMavenUpdate twice in a row should yield the same results, always. Else, please change the method name to indicate it does more than checking.

fbricon · 2018-03-27T13:43:24Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

@@ -25,24 +39,84 @@
 *
 */
 public class MavenBuildSupport implements IBuildSupport {
+	private Map<String, String> pomDigests;
+
+	private static final String POM_SERLIZATION_FILE_NAME = ".pom-digests";


POM_SERIALIZATION_FILE_NAME

fbricon · 2018-03-27T13:44:23Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

+		try (ObjectOutputStream outStream = new ObjectOutputStream(new FileOutputStream(f))) {
+			outStream.writeObject(pomDigests);
+		} catch (IOException e) {
+			JavaLanguageServerPlugin.logException("Exception occured while serizalizion of pom digests", e);


while serializing pom digests

fbricon · 2018-03-27T13:44:48Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupport.java

+		try (ObjectInputStream ois = new ObjectInputStream(new FileInputStream(f))) {
+			return (Map<String, String>) ois.readObject();
+		} catch (IOException | ClassNotFoundException e) {
+			JavaLanguageServerPlugin.logException("Exception occured while deserizalizion of pom digests", e);


while deserializing pom digests

fbricon · 2018-03-27T13:48:33Z

Ok so one major problem with this approach is we only serialize poms when a file change is detected. We should fingerprint them before importing them too, else you'll import the project, save a pom.xml without changing it, it'll be detected as an actual change and update will run

Signed-off-by: Dmitrii Bocharov <[email protected]>

Signed-off-by: Snjezana Peco <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]>

fbricon · 2018-04-11T12:22:42Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/DigestStore.java

+ *
+ */
+public class DigestStore {
+	private Map<String, String> pomDigests;


fileDigests

eventually we could use the same mechanism for Gradle. Since this class has no coupling to Maven, I'd prefer we treat it as file-type agnostic.

fbricon · 2018-04-11T14:25:41Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/DigestStore.java

+
+	private static final String POM_SERIALIZATION_FILE_NAME = ".pom-digests";
+
+	public DigestStore(Plugin plugin) {


You don't need a plugin object. Storage location path should be enough

fbricon · 2018-04-11T14:26:21Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaLanguageServerPlugin.java

@@ -121,6 +123,7 @@ public void start(BundleContext bundleContext) throws BackingStoreException {
 		JavaManipulation.setPreferenceNodeId(PLUGIN_ID);
 		preferenceManager = new PreferenceManager();
 		initializeJDTOptions();
+		digestStore = new DigestStore(this);


new DigestStore(getStateLocation())

Signed-off-by: Thomas Mäder <[email protected]>

fbricon · 2018-04-13T13:30:14Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/DigestStore.java

+
+	}
+
+	private void serializePomDigests() {


serializeFileDigests

fbricon · 2018-04-13T13:30:25Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/DigestStore.java

+	}
+
+	@SuppressWarnings("unchecked")
+	private Map<String, String> deserializePomDigests() {


deserializeFileDigests

Signed-off-by: Thomas Mäder <[email protected]>

fbricon · 2018-04-13T23:22:51Z

squash merged as 6c64df5

bdshadow force-pushed the update_project_if_needed branch 2 times, most recently from 9011e8c to 7dfdb92 Compare February 10, 2018 19:40

bdshadow mentioned this pull request Feb 10, 2018

Determine Che-in-Che Resource Requirements with jdt.ls eclipse-che/che#8335

Closed

bdshadow force-pushed the update_project_if_needed branch from 26db36f to e190b15 Compare February 12, 2018 15:27

fbricon reviewed Feb 15, 2018

View reviewed changes

bdshadow force-pushed the update_project_if_needed branch from e190b15 to f2a8168 Compare February 19, 2018 11:10

fbricon reviewed Feb 26, 2018

View reviewed changes

fbricon reviewed Mar 26, 2018

View reviewed changes

tsmaeder requested changes Mar 27, 2018

View reviewed changes

fbricon requested changes Mar 27, 2018

View reviewed changes

fbricon reviewed Mar 27, 2018

View reviewed changes

tsmaeder force-pushed the update_project_if_needed branch from 26d4698 to acfbc4b Compare April 6, 2018 10:48

bdshadow and others added 7 commits April 11, 2018 13:32

update project only if pom changed, based on md5

3bb7a43

Signed-off-by: Dmitrii Bocharov <[email protected]>

fix concurrency and update digest on new values

45a2c87

Signed-off-by: Dmitrii Bocharov <[email protected]>

force update if project is updated intentionally

93a13e7

Signed-off-by: Dmitrii Bocharov <[email protected]>

add serialization of pom digests

1c56ca3

Signed-off-by: Dmitrii Bocharov <[email protected]>

update javadoc of IBuildSupport

1ba0321

Signed-off-by: Dmitrii Bocharov <[email protected]>

Process non-java files

b23df6b

Signed-off-by: Snjezana Peco <[email protected]>

Simplify comparison

f456c53

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder added 5 commits April 11, 2018 13:32

Addressed PR comments

14fdfce

Signed-off-by: Thomas Mäder <[email protected]>

Java style is not to use "this" to qualify instance vars.

546de0e

Signed-off-by: Thomas Mäder <[email protected]>

Rely on importer to update maven projects not seen before.

b1e11d1

Signed-off-by: Thomas Mäder <[email protected]>

Fix typos

d014da7

Signed-off-by: Thomas Mäder <[email protected]>

Extract DigestStore it and check when importing

8444b87

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder force-pushed the update_project_if_needed branch from acfbc4b to 8444b87 Compare April 11, 2018 11:34

fbricon reviewed Apr 11, 2018

View reviewed changes

Addresssed PR comments

d25d6e0

Signed-off-by: Thomas Mäder <[email protected]>

fbricon reviewed Apr 13, 2018

View reviewed changes

Remove mention of poms

fad9235

Signed-off-by: Thomas Mäder <[email protected]>

fbricon closed this Apr 13, 2018

fbricon added this to the Mid April 2018 milestone Apr 13, 2018


		private static final String POM_SERIALIZATION_FILE_NAME = ".pom-digests";

		public DigestStore(Plugin plugin) {

Update project if needed #547

Update project if needed #547

Conversation

bdshadow commented Feb 10, 2018

fbricon commented Feb 12, 2018

fbricon commented Feb 12, 2018

bdshadow commented Feb 12, 2018

bdshadow commented Feb 15, 2018

fbricon commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon commented Apr 13, 2018

fbricon Feb 26, 2018 •

edited

Loading

tsmaeder Mar 27, 2018 •

edited

Loading

tsmaeder Mar 29, 2018 •

edited

Loading

fbricon Mar 27, 2018 •

edited

Loading