Quick-fix for Import-Package resolution failure due to missing mandatory attributes #678

alshamams · 2023-07-23T14:19:55Z

When a package is imported that has been exported with mandatory attributes, following are the possibilities:

the import package is missing a mandatory attribute,
the package has a mandatory attribute, but it does not match the importing package's export declaration verb,
it has a mandatory attribute with the matching key, but there is a value mismatch

This commit aims to provide a quick-fix that queries the mandatory attributes and supplies them at the import site.

Fixes: #578

github-actions · 2023-07-23T14:44:56Z

Test Results

255 files - 29 255 suites - 29 31m 6s ⏱️ - 12m 24s
900 tests - 2 627 864 ✅ - 2 603 34 💤 - 24 2 ❌ ±0
2 454 runs - 5 646 2 373 ✅ - 5 585 75 💤 - 65 6 ❌ +4

For more details on these failures, see this check.

Results for commit aabd31d. ± Comparison against base commit 1d5bd08.

This pull request removes 2627 tests.

AllPDETests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test2
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test3
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test4
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test5
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test6
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test7
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test2
…

♻️ This comment has been updated with latest results.

HannesWell · 2023-07-24T06:21:40Z

Thank you @alshamams.
From a very coarse look this seems fine.

I'll performe a more detailed review this/tomorrow evening.
Everyoneelse' review is of course welcome. :)

alshamams · 2023-07-24T07:07:39Z

Thank you @alshamams. From a very coarse look this seems fine.

I'll performe a more detailed review this/tomorrow evening. Everyoneelse' review is of course welcome. :)

alshamams · 2023-07-24T07:08:11Z

Thank you @alshamams. From a very coarse look this seems fine.

I'll performe a more detailed review this/tomorrow evening. Everyoneelse' review is of course welcome. :)

Thank you @HannesWell !

vik-chand · 2023-07-24T07:55:00Z

I think closed by mistake

ahmed82

lgtm

HannesWell · 2023-08-01T22:34:55Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/BundleErrorReporter.java

+								String[] mandatories = (String[]) exportDirectives.get(Constants.RESOLUTION_MANDATORY);
+								for (String mandatory : mandatories) {
+									if (importAttributes == null || !importAttributes.containsKey(mandatory)
+											|| importAttributes.get(mandatory).toString() != exportAttributes


You should (almost) never compare strings by their reference, since it is easy to have different string object with the same content.

HannesWell

I looked into this and in general this worked, nice job so far.
But there were a few places that needed fine-tuning (e.g. enhanced messages) or could be simplified/imported. I pushed another commit with those suggests.

What's currently missing is the handling in case there are multiple providers that export a package but with different attribute values.
In my suggestion commit I already made a change to get all exporters of a package, but it still has to be implemented that those multiple providers are considered in the error resolution.

This should be combined with the consideration of multiple mandatory/not-mandatory attributes.
I think it would be best to iterate over all exporters and create one resolution per exporter that each sets all mandatory attributes to the exporters value and set all non-mandatory attributes to the exporter's value, if the importer has another value.

Besides that a few test cases would be good. They could probably be added to BundleErrorReporterTest.

laeubi · 2023-08-02T05:43:41Z

I think it would be great if this is already offered at the time when importing the package, maybe also add support for the import version as well.

HannesWell · 2023-08-31T22:46:57Z

I think it would be great if this is already offered at the time when importing the package, maybe also add support for the import version as well.

That would be great but also would make this task much greater since the proposal would need to be enhanced as well. We could certainly do that, but I suggest we keep this focused on enhancing the error messages/quick-fix and leave the enhancement of the Import-Package proposals for a follow up.
Not suggesting to add the attributes initially also has the 'advantage' that users are very aware that they add a package-import with attributes which is usually a sign that something special is happening.

alshamams · 2023-09-05T18:50:33Z

Hi @HannesWell ,
I have incorporated the changes as per your suggestion. Kindly look through and let me know.

Can you also have a look at the resolution window for the Quick Fix and suggest possible improvements, if any? Thanks in advance.

HannesWell · 2023-09-05T22:37:49Z

Thanks. I will review it as soon as possible (probably in the next days).

laeubi · 2023-09-06T03:41:42Z

We could certainly do that, but I suggest we keep this focused on enhancing the error messages/quick-fix and leave the enhancement of the Import-Package proposals for a follow up.

Sure but I think the main problem is that an invalid package import is added at the first place.

@alshamams you should rebase (and maybe squash) your commits here.

alshamams · 2023-10-03T08:03:51Z

Hi @laeubi , @HannesWell,
I have just rebased my commit. Kindly review and let me know.

HannesWell · 2023-10-04T23:26:56Z

Hi @laeubi , @HannesWell, I have just rebased my commit. Kindly review and let me know.

Sorry for the delay. Will review this ASAP.

alshamams · 2023-11-06T07:05:20Z

Hi @laeubi , @HannesWell,
Just wanted to remind you that I await your review comments. Do let me know.🙂

attributes:

HannesWell self-requested a review July 24, 2023 06:21

alshamams closed this Jul 24, 2023

vik-chand reopened this Jul 24, 2023

ahmed82 reviewed Jul 28, 2023

View reviewed changes

HannesWell force-pushed the manifest_bund_add branch from caf4ec5 to 5e44df0 Compare August 1, 2023 22:29

HannesWell reviewed Aug 1, 2023

View reviewed changes

HannesWell requested changes Aug 1, 2023

View reviewed changes

HannesWell mentioned this pull request Aug 31, 2023

org.eclipse.m2e.archetype.common listed as fragment in m2e.feature eclipse-m2e/m2e-core#1537

Open

alshamams force-pushed the manifest_bund_add branch from 8ff8263 to 7a21d1c Compare September 6, 2023 10:44

alshamams force-pushed the manifest_bund_add branch from 7a21d1c to 8a7cad3 Compare October 3, 2023 08:01

alshamams force-pushed the manifest_bund_add branch from 8a7cad3 to 76b29c6 Compare November 13, 2023 13:17

alshamams force-pushed the manifest_bund_add branch from 76b29c6 to 2cce3ce Compare December 1, 2023 07:06

laeubi force-pushed the manifest_bund_add branch from 2cce3ce to aa3c4d8 Compare January 27, 2024 15:59

laeubi assigned HannesWell Jan 27, 2024

Quick-fix for import package resolution failure due to missing mandatory

aabd31d

attributes:

laeubi force-pushed the manifest_bund_add branch from aa3c4d8 to aabd31d Compare February 11, 2024 06:39

laeubi requested a review from HannesWell February 11, 2024 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick-fix for Import-Package resolution failure due to missing mandatory attributes #678

Quick-fix for Import-Package resolution failure due to missing mandatory attributes #678

alshamams commented Jul 23, 2023

github-actions bot commented Jul 23, 2023 •

edited

Loading

HannesWell commented Jul 24, 2023 •

edited

Loading

alshamams commented Jul 24, 2023

alshamams commented Jul 24, 2023

vik-chand commented Jul 24, 2023

ahmed82 left a comment

HannesWell Aug 1, 2023

HannesWell left a comment

laeubi commented Aug 2, 2023

HannesWell commented Aug 31, 2023

alshamams commented Sep 5, 2023

HannesWell commented Sep 5, 2023

laeubi commented Sep 6, 2023

alshamams commented Oct 3, 2023

HannesWell commented Oct 4, 2023

alshamams commented Nov 6, 2023

Quick-fix for Import-Package resolution failure due to missing mandatory attributes #678

Are you sure you want to change the base?

Quick-fix for Import-Package resolution failure due to missing mandatory attributes #678

Conversation

alshamams commented Jul 23, 2023

github-actions bot commented Jul 23, 2023 • edited Loading

Test Results

HannesWell commented Jul 24, 2023 • edited Loading

alshamams commented Jul 24, 2023

alshamams commented Jul 24, 2023

vik-chand commented Jul 24, 2023

ahmed82 left a comment

Choose a reason for hiding this comment

HannesWell Aug 1, 2023

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

laeubi commented Aug 2, 2023

HannesWell commented Aug 31, 2023

alshamams commented Sep 5, 2023

HannesWell commented Sep 5, 2023

laeubi commented Sep 6, 2023

alshamams commented Oct 3, 2023

HannesWell commented Oct 4, 2023

alshamams commented Nov 6, 2023

github-actions bot commented Jul 23, 2023 •

edited

Loading

HannesWell commented Jul 24, 2023 •

edited

Loading