-
Notifications
You must be signed in to change notification settings - Fork 83
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
quick-fix for removing require-bundle: fault-lines do not match resolution batch #685
Comments
@gireeshpunathil May be I did not understand below points.. What do you mean by non-existent bundles ? "Add.." button allows only existing bundles to select and add . "error(s)" - where to look for it ? - in problems view ? |
@lathapatil - sorry if it confused you.
the bundle |
as per my understanding of the multi-fix, line 79 and 80 appearing the list is fine - as those two share same problem: even if user has selected quick-fix for one of the errors from the workspace.
Yes. because that is what multi-fix should do: (obviously, only if user selects all the items - like in your case, line 79 and 80 are selected) |
This is working fine in below Eclipse SDK version and also in runtime from my workspace setup I can see this issue only when I try these steps inside the workspace for which we can not debug and fix. But, I could see different issue in above mentioned SDK and as well as in runtime i.e. when its combination of both import-package and require-bundle as below . It removes/fixes the bundle only on which I right clicked and asked for quick-fix. @gireeshpunathil Is it expected to remove all 4 issues in this case as well ? |
@lathapatil - this looks like a different variation to me; but you are right in that this is problematic too.
so to answer your question:
|
This is working fine in latest SDK .. You can verify once. |
Then, is this the actual fix required here - to appear only 2 lines instead of 4 lines ? |
@gireeshpunathil I have analyzed the case and the root cause for this is having same marker attribute for both kind of problems. require-bundle problem and Import-package problem both are having same compilerKey (compilers.p.unresolved-import) as one of the marker attribute based on which all 4 problems are listed . Should we change the compilerKey for the require-bundle problem ? The list of compilerKey are available in CompilerFlags java class. |
@lathapatil - thanks for looking into this, and yes - your findings look promising! let us go that route and see what happens. let me know when you have pushed a patch, I will be happy to review and test! |
Do you suggest creating a new key in CompilerFlags class for unresolved bundle problem of require-bundle header of manifest file ? I see even for Fragment-Host and export-package same compiler key ( |
I don't see an existing relevant key in the file, so let us create a new one for unresolved bundle for require-bundle. |
I see this is also an issue .. because export-package problem is also listed along with others mentioned earlier because of same complierKey |
tbh, I don't know about that, let us ask @HannesWell ! Hannes - do you have a recommendation on potential issues with reuse of keys for different problem types in general, and for fragment-host and unresolved-imports in particular? |
I'm not so deep in the topic of problem-markers, but I agree that using the same problem maker is probably a problem when using multi-fixes. From that point of view I think, yes it makes sense to have dedicated markers for each kind of problem (import-package, require-bundle, fragment-host). And about the example where
This just needs to be handled like one header whose values are split over multiple lines, but some line contain multiple entries:
What also has to be considered is that |
Usually it is enough to have additional attributes and not new marker types. A type is more like "it is a java problem", or "it is a manifest problem" or similar...
If I remember correctly this preference was a bit confusing in the past, so if it could be made more specific it is good I think. |
Thanks for the detailed explanation on why a new complier-key is not required and from this I understand that behavior explained in the comment is valid where in missing bundles and imports are considered for a common fix . There is another issue persists which explained here..
to reproduce this ..
Expected behavior is to fix all what user have selected. Root cause - Code assumes that only "missing bundle" problems have come for fix and search for bundle-id attribute to remove/fix it where as "missing imports" have different attribute for it i.e. "package-name" This has to be fixed here at least. |
@HannesWell do you think it has to be fixed here ? |
Brief analysis of the issue is here . When it comes to pde.. it is invoking createChange method of RemoveRequireBundleResolution.java for multiple relevant markers (remove require bundle , remove import package , remove export package etc..) which works only for remove require bundle only in this example and hence other markers are not resolved. possible solution could be - using MultiFixResolution .. but probably adding below piece of code(the first if condition in the method) in every resolution class may not be good
Alternative solution could be modifying the caller - processResolution (eclipse.platform.ui) so as to call respective resolution class for each marker which are considered for common fix based on compiler key. Which approach is better for this fix ? Second approach needs code change in eclipse.platform.ui |
thanks @lathapatil for the detailed analysis! I like the first approach. I would assume there are only a handful of resolution managers that need this change, so it should be fine. thanks again! |
As of now I have tested only for Remove require bundle resolution along with that I had added errors related to unresolved import and export packages. I do not have an idea on working of all other resolution classes available in PDE. So I am unsure of which all classes I can update with this multi-fix resolution. In general all resolution classes implementing AbstractManifestMarkerResolution class? |
why don't you just go ahead with remove-require-bundle alone in a PR? makes your code development easy - also code reviews! |
That sounds good as per the issue description but the quick fix still fails if user want to fix other issues. (Example : when user wants to fix Unresolved import and other related errors it fails for other related errors unless we add Multi-fix resolution code in RemoveImportPackageResolution as well.. ) or may be I can fix in the classes which I know it would work ? |
steps to recreate:
remove bundle from the list
optionobserve only the first item in the list as removed. Expectation is to remove all the selected bundles.
The text was updated successfully, but these errors were encountered: