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.

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

Open
gireeshpunathil opened this issue Jul 28, 2023 · 23 comments · May be fixed by #1196
Open

quick-fix for removing require-bundle: fault-lines do not match resolution batch #685

gireeshpunathil opened this issue Jul 28, 2023 · 23 comments · May be fixed by #1196

Comments

@gireeshpunathil
Copy link
Contributor

steps to recreate:

  • create a simple plugin project
  • add a few non-existent bundles under require-bundle header in the manifest
  • right click on the error(s) and select quick-fix
  • select remove bundle from the list option
  • select all the applicable lines from the below list and finish

observe only the first item in the list as removed. Expectation is to remove all the selected bundles.

@lathapatil
Copy link
Contributor

@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 ?

@gireeshpunathil
Copy link
Contributor Author

@lathapatil - sorry if it confused you.

  • open the manifest file
  • hand-write some non-existing bundles: see the example below:

image

the bundle foo.bar does not exist. I added it manually.

@lathapatil
Copy link
Contributor

The expected behavior is to remove all selected bundles or it should only display related bundles in the list for user to select ?

Example in below screenshot should it display only for line number 79 on clicking "Remove bundle from list" ? because line number 80 belongs to different bundle.

QuickFix_pde_issue

@gireeshpunathil
Copy link
Contributor Author

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.

The expected behavior is to remove all selected bundles or it should only display related bundles in the list for user to select ?

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)

@lathapatil
Copy link
Contributor

lathapatil commented Oct 27, 2023

This is working fine in below Eclipse SDK version
Version: 2023-12 (4.30)
Build id: I20231024-1800

and also in runtime from my workspace setup
Version: 2023-12 M1 (4.30.0 M1)
Build id: 20231005-1720)

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.
Here it fixed the issue on line 7 (Require-Bundle) only !

2023-10-27_15h44_16

2023-10-27_15h45_15

@gireeshpunathil Is it expected to remove all 4 issues in this case as well ?

@gireeshpunathil
Copy link
Contributor Author

@lathapatil - this looks like a different variation to me; but you are right in that this is problematic too.

  • line 6 - unknown package, line 7 - unknown dependency: they cannot have a common fix, but the quick-fix windows suggests / lists those 2 together!
  • even for issues with a common fix (such as 2 issues in line 7), second user selection gets ignored (number of check-boxes): that was my original issue reported here. I haven't checked with the latest to see if it is vanished or not.

so to answer your question: Is it expected to remove all 4 issues in this case as well ?

  • no, appearance of the 4 lines is wrong in the first place, as they don't have a common quick-fix
  • suppose only 2 of those are appearing (say 2 for line 7 or 2 line 6 but not for both), and if user selects both the tick-boxes, then yes: remove both (which does not happen right now)

@lathapatil
Copy link
Contributor

  • suppose only 2 of those are appearing (say 2 for line 7 or 2 line 6 but not for both), and if user selects both the tick-boxes, then yes: remove both (which does not happen right now)

This is working fine in latest SDK .. You can verify once.

@lathapatil
Copy link
Contributor

  • no, appearance of the 4 lines is wrong in the first place, as they don't have a common quick-fix

Then, is this the actual fix required here - to appear only 2 lines instead of 4 lines ?

@lathapatil
Copy link
Contributor

lathapatil commented Jan 4, 2024

  • no, appearance of the 4 lines is wrong in the first place, as they don't have a common quick-fix

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.

@gireeshpunathil
Copy link
Contributor Author

@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!

@lathapatil
Copy link
Contributor

lathapatil commented Jan 4, 2024

@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 ?
or
are there any relevant key already available in the above mentioned class?

I see even for Fragment-Host and export-package same compiler key ( CompilerFlags.P_UNRESOLVED_IMPORTS ) is used

@gireeshpunathil
Copy link
Contributor Author

I don't see an existing relevant key in the file, so let us create a new one for unresolved bundle for require-bundle.

@lathapatil
Copy link
Contributor

I see even for Fragment-Host and export-package same compiler key ( CompilerFlags.P_UNRESOLVED_IMPORTS ) is used

I see this is also an issue .. because export-package problem is also listed along with others mentioned earlier because of same complierKey

@gireeshpunathil
Copy link
Contributor Author

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?

@HannesWell
Copy link
Member

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).
On the other hand, what are the possible actions associated with the problem? I only see REMOVE, MARK OPTIONAL, CONFIGURE_TP (to add it) and except mark optional that cannot be applied to Fragment-Host headers, all other actions can be applied to all of the mentioned headers. With that in mind I think it is fine to share the key.

And about the example where Import-Package and Require-Bundle values are missing

Import-Package: missing.pack1, missing.pack2
Require-Bundle: missing.bundle1, missing.bundle2

This just needs to be handled like one header whose values are split over multiple lines, but some line contain multiple entries:

Require-Bundle: missing.bundle1, missing.bundle2
 missing.bundle3, missing.bundle4

What also has to be considered is that CompilerFlags.P_UNRESOLVED_IMPORTS is used as key in the preferences. And I don't think it is necessary to be able to configure the severity of missing Require-Bundle, Import-Package etc. separately. Of course the preference key and problem-marker key are in general independent although they have the same text.
But with the considerations from above in mind, I don't think another marker-key is necessary too.

@laeubi
Copy link
Contributor

laeubi commented Jan 5, 2024

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...

And I don't think it is necessary to be able to configure the severity of missing Require-Bundle, Import-Package etc. separately.

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.

@lathapatil
Copy link
Contributor

lathapatil commented Jan 5, 2024

This just needs to be handled like one header whose values are split over multiple lines, but some line contain multiple entries:

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..

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. Here it fixed the issue on line 7 (Require-Bundle) only !

to reproduce this ..

  1. Make sure problems view is having both kind of errors - missing bundle and missing import
  2. right click on missing bundle error and quick fix
  3. Quick fix wizard opens and select "Remove bundle from the list" option
  4. Displays both missing bundle and missing import for selection to remove
  5. select all and finish
  6. Removes/fixes only missing bundles but not missing imports

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.

@lathapatil
Copy link
Contributor

This has to be fixed here at least.

There is another issue persists which explained here..

@HannesWell do you think it has to be fixed here ?

@lathapatil
Copy link
Contributor

lathapatil commented Feb 20, 2024

Brief analysis of the issue is here .
Multi-fix resolution is currently not working on quick fix because the caller - processResolution (eclipse.platform.ui) is invoking a resolution class (for remove require bundle -RemoveRequireBundleResolution.java )on which user have selected for quick-fix and other markers sent along with this will not be resolved.

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

@Override
	protected void createChange(BundleModel model) {
		fBundleId = marker.getAttribute("bundleId", null); //$NON-NLS-1$
		if (fBundleId == null) {
			MultiFixResolution multiFixResolution = new MultiFixResolution(marker, null);
			multiFixResolution.run(marker);
		}
		Bundle bundle = (Bundle) model.getBundle();
		RequireBundleHeader header = (RequireBundleHeader) bundle.getManifestHeader(Constants.REQUIRE_BUNDLE);
		if (header != null)
			header.removeBundle(fBundleId);
	}

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.
This can be achieved by writing the code to determine resolution class based on problem-ID attribute in the marker and invoke respective resolution class for each marker. [Similar implementation can be seen in ResolutionGenerator class ]

Which approach is better for this fix ? Second approach needs code change in eclipse.platform.ui

@gireeshpunathil
Copy link
Contributor Author

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!

@lathapatil
Copy link
Contributor

I would assume there are only a handful of resolution managers that need this change

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?
or
resolution classes for which multi-fix has been implemented here might make sense ?

@gireeshpunathil
Copy link
Contributor Author

why don't you just go ahead with remove-require-bundle alone in a PR? makes your code development easy - also code reviews!

@lathapatil
Copy link
Contributor

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants