Community
Participate
Working Groups
Is it clear we want to put this off ... to Luna? Not sure it'd qualify for "service release", technically, so not sure how long you/we are willing to live with it. I don't personally care ... just trying to be explicit. Thanks,
Guess I should have read "depends on" bug first :) ... I see you have removed it from PDE, so would be easy to add in "service stream".
Curtis, if you prepare a patch, then I'll take a look and we can put it into Kepler.
Created attachment 231051 [details] Incomplete fix This patch adds the pom version tool to releng tools. However, it has a few missing pieces still. 1) It doesn't get the version out of the manifest We used to do this using the PDE model. Now we'll have to look at the file ourselves, though we can use Equinox to parse the file. 2) No way to add the builder This was a major bonus of extending the PDE builder. We didn't have to modify every project to enable it. 3) Markers aren't showing up when created Probably some simple mistake I made in defining the marker 4) Strings copied from PDE haven't been NLS'd.
http://git.eclipse.org/c/platform/eclipse.platform.releng.git/?h=cwindatt%2F408152_PomVersionTool Created a topic branch for the current work.
Created attachment 231122 [details] continued work This patch moves the builder to be a resource listener. There are still some issues: 1. the marker char start / end are off by two chars 2. the 'builder' does not run until the bundle loads - unlike a traditional builder that will be loaded automatically 3. using an IWorkspaceRunnable (or not) causes 'resource tree locked' exceptions 4. when the bundle is loaded, do we want to run a 'build' of the whole workspace to check if anything has changed while it was not loaded?
(In reply to comment #6) > Created attachment 231122 [details] [diff] > continued work > > This patch moves the builder to be a resource listener. There are still some > issues: > 1. the marker char start / end are off by two chars No idea why, but the char array passed to #characters starts with \r\n, which gives the off by 2 failure I guess. > 2. the 'builder' does not run until the bundle loads - unlike a traditional > builder that will be loaded automatically This is a hard one to tackle. We could add the org.eclipse.ui.startup extension but that's not something that "normal" users of the Releng plug-in want. I guess the easiest would be to put the whole feature into its own plug-in and auto-start it. People who install that bundle are OK that it gets loaded. > 3. using an IWorkspaceRunnable (or not) causes 'resource tree locked' > exceptions Yes, that's the downside of using a the resource listener instead of a builder. You have to put the marker modifications into an asyncExec. I would rearrange the code, so that you only have one runnable that deletes and creates the marker (if needed). IMPORTANT NOTE: Even then your code will not work because IPomVersionConstants.PROBLEM_MARKER_TYPE is not correct: it misses a dot. > 4. when the bundle is loaded, do we want to run a 'build' of the whole > workspace to check if anything has changed while it was not loaded? I would only do this exactly once at the first start (remember in a preference). When changing the preference from Ignore to something else, we also need to do it. The resource delta visitor needs work: - It must only validate when the file actually changed. Currently, it re-validates due to the marker deltas. See #getFlags() for details. - It needs to validate when the pom file changes (the user could fix the version in the pom, in which case the marker needs to be deleted) - We need to validate when the manifest gets deleted. - I would do the work (validate) directly in the visitor and not add additional state (fResource). - IFile manifestFile = file.getProject().getFile(MANIFEST_PATH); if (file.equals(manifestFile)){ This is too expensive and creates file instances. faster is: delta.getProjectRelativePath().equals(MANIFEST_PATH) Even faster would be to use #segments() and compare the corresponding values, but that's probably overkill. Also, checking the resource type is faster than checking the derived state ==> do this check first.
> I guess the easiest would be to put the whole feature into its own plug-in and > auto-start it. People who install that bundle are OK that it gets loaded. On a second thought, I think it's good enough to just auto-start the Releng plug-in. It's not that big and I don't think there are many users out there using it.
> Yes, that's the downside of using a the resource listener instead of a > builder. You have to put the marker modifications into an asyncExec. There's a simpler solution than asyncExec: register the resource listener as POST_BUILD listener. During that notification, modifications are allowed. See also org.eclipse.ui.texteditor.MarkerUtilities.
Created attachment 231273 [details] update Here is an updated patch that: 1. improves the delta visitor (before it was just a copy from the PDE builder) 2. auto-starts the bundle 3. fixes the marker issues 4. scans projects that are opened (and that have the PDE nature)
Created attachment 231276 [details] update 2 The last patch was missing deletions, so I committed the fix to my local repo and regenerated the patch
Comment on attachment 231276 [details] update 2 Pushed Mike's patch to the topic branch http://git.eclipse.org/c/platform/eclipse.platform.releng.git/?h=cwindatt%2F408152_PomVersionTool
(In reply to comment #11) > Created attachment 231276 [details] > update 2 > > The last patch was missing deletions, so I committed the fix to my local > repo and regenerated the patch After loading up the patch Curtis found that I was using 1.5 methods Integer.valueOf(int). I was getting no compile errors (because there is no 1.4 JRE on Mac anymore and I had a 1.4 ee file that pointed to the 1.7 libs) - which got me thinking, should we just use a higher BREE? like 1.6, 1.7? Is there a reason to keep the tools on 1.4?
(In reply to comment #13) > - which got me thinking, should we just use a higher BREE? like 1.6, 1.7? Is > there a reason to keep the tools on 1.4? Let's not start to mix things. If you want to increase the BREE, please file a new bug which we can discuss/address in 4.4. In my workspace the code does not work (exceptions) because it uses the 2.0 style startup extension construct which relies on the compatibility layer: " If the extension does not provide a class as an attribute on the startup element, the plug-in's activator (plug-in class) must implement org.eclipse.ui.IStartup. Note that this form is deprecated and should no longer be used. Its functioning relies on the availability of the org.eclipse.core.runtime.compatibility plug-in and the org.eclipse.core.runtime.compatibility.registry fragment. Plug-ins that provide an extension to this extension point are listed in the workbench preferences and the user may disable any plug-in from early startup. " ==> add RelEngPluginEarlyStartup and refer it from the extension. Also, we should not remove the lazy startup activation. This can cause ugly bugs if it gets missed should we decide to (re-)move the pom updater at a later point. Why did you set an explicit UTF-8 encoding on PomVersionErrorReporter? This seems strange and should be removed. PomVersionMarkerResolution has one last non-externalized string. org.eclipse.releng.tools.pomversion should be exported The default severity should be 'Ignore' for two reasons: - No behavior change for "normal" users of the Releng plug-in. - The whole workspace is checked when the user changes the severity. Otherwise, only touched manifest/pom files will be checked. I've fixed all above issues, did some minor tweaks here and there, and released the code to master with Mike as author with: http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/?id=d3b01514b66ccc887e0b7fcb32642ac11dad07b9 Please review again and verify that it works.
Made the new package internal with Fixed with http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/?id=eaa82d2238b52762d5b3aab9a1fbc8c363a0ab1a
(In reply to comment #14) > The default severity should be 'Ignore' for two reasons: > - No behavior change for "normal" users of the Releng plug-in. > - The whole workspace is checked when the user changes the severity. > Otherwise, > only touched manifest/pom files will be checked. While the second point will cause some odd behaviour, I disagree that the default severity should be ignore. The target audience of the releng tools should have this tool turned on always. I would also expect most developers to not have any bad pom versions in their workspace currently. So not doing a full workspace check is fine as long as they get a warning marker after upping the manifest version (or better yet an error marker). > > I've fixed all above issues, did some minor tweaks here and there, and > released the code to master with Mike as author with: > http://git.eclipse.org/c/platform/eclipse.platform.releng.git/commit/ > ?id=d3b01514b66ccc887e0b7fcb32642ac11dad07b9 > > > Please review again and verify that it works. 1) First time I tried the marker resolution the marker went away, but the editor contents did not change (including the error marker underline). Closing and reopening the editor didn't fix this. However, after a restart everything was correct and I couldn't reproduce. 2) Without any noticeable progress it is hard to know when the listener will run. Sometimes I change the version and the marker is added or updated instantly. Other times it may wait a while (seems to be when the project is building, which is slow if you are modifying the plug-in version).
(In reply to comment #16) > (In reply to comment #14) > > The default severity should be 'Ignore' for two reasons: > > - No behavior change for "normal" users of the Releng plug-in. > > - The whole workspace is checked when the user changes the severity. > > Otherwise, > > only touched manifest/pom files will be checked. > > While the second point will cause some odd behaviour, I disagree that the > default severity should be ignore. The target audience of the releng tools > should have this tool turned on always. I would also expect most developers > to not have any bad pom versions in their workspace currently. So not doing > a full workspace check is fine as long as they get a warning marker after > upping the manifest version (or better yet an error marker). I'd be OK to change it back to 'Error' if you implement what I said in comment 7: > 4. when the bundle is loaded, do we want to run a 'build' of the whole > workspace to check if anything has changed while it was not loaded? I would only do this exactly once at the first start (remember in a preference). ==> do it in earlyStartup(), which will be called after the normal start().
