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

Import-Package: org.junit.vintage.engine does not work #574

Closed
cdietrich opened this issue Apr 13, 2023 · 21 comments
Closed

Import-Package: org.junit.vintage.engine does not work #574

cdietrich opened this issue Apr 13, 2023 · 21 comments

Comments

@cdietrich
Copy link
Contributor

cdietrich commented Apr 13, 2023

Import-Package: org.junit.vintage.engine does not work
(although the dialog proposes it)

what could be the problem?

Bildschirmfoto 2023-04-13 um 14 33 42

Bildschirmfoto 2023-04-13 um 14 35 32

@iloveeclipse
Copy link
Member

I've just created default plugin project with wizard and got this resolved without issues:

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: AAA
Bundle-SymbolicName: AAA
Bundle-Version: 1.0.0.qualifier
Require-Bundle: junit-vintage-engine;bundle-version="5.9.2"
Automatic-Module-Name: AAA
Bundle-RequiredExecutionEnvironment: JavaSE-17

However I use full / latest SDK nightly build as running platform. Not sure which Eclipse version you have and which target platfrom you use.

@cdietrich
Copy link
Contributor Author

cdietrich commented Apr 13, 2023

i see the problem in 2023-03 (dsl package) with 2023-03 as target platform. (aka no explicit target set)
will test with m1 once it is out

@LorenzoBettini
Copy link

I've just created default plugin project with wizard and got this resolved without issues:

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: AAA
Bundle-SymbolicName: AAA
Bundle-Version: 1.0.0.qualifier
Require-Bundle: junit-vintage-engine;bundle-version="5.9.2"
Automatic-Module-Name: AAA
Bundle-RequiredExecutionEnvironment: JavaSE-17

However I use full / latest SDK nightly build as running platform. Not sure which Eclipse version you have and which target platfrom you use.

That's a required bundle, though: in our case it's an imported package.

@cdietrich
Copy link
Contributor Author

yes require bundle works perfectly fine
but the bundle name is not stable
for the "platform consuming maven thing" accross platform versions

@iloveeclipse
Copy link
Member

If someone would have provided the manifest as a text, I would not made the mistake to import bundle :-)

Here is it and yes, it is broken on latest 4.28 SDK I build too:

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: AAA
Bundle-SymbolicName: AAA
Bundle-Version: 1.0.0.qualifier
Import-Package: org.junit.vintage.engine;version="5.9.2"
Automatic-Module-Name: AAA
Bundle-RequiredExecutionEnvironment: JavaSE-17

@HannesWell : I believe you were involved in some PDE dependency management refactorings: could you please take a look?

@iloveeclipse
Copy link
Member

Note: the package export is not a "usual" one one would expect, it looks like:

org.junit.vintage.engine;version="5.9.2";status=INTERNAL;mandatory:=status

see below

image

@HannesWell
Copy link
Member

@HannesWell : I believe you were involved in some PDE dependency management refactorings: could you please take a look?

Yes I can have a look this evening.

@mickaelistria
Copy link
Contributor

Indeed, as @iloveeclipse mentioend, the Import-Pacakge requires to use the status=INTERNAL property (I don't remember the exact syntax, if it's = or :=, whether and where quotes are necessary...) to allow resolving against the upstream org.junit.vintage.engine.

@cdietrich
Copy link
Contributor Author

Import-Package: org.junit;version="4.13.2",
 org.junit.jupiter.api;version="5.9.2",
 org.junit.platform.launcher;version="1.9.2",
 org.junit.vintage.engine;status=INTERNAL;version="5.9.2"

seems to work, yes

@iloveeclipse
Copy link
Member

Can anyone explain for what org.junit.vintage.engine;status=INTERNAL;version="5.9.2" is good for?

@merks
Copy link
Contributor

merks commented Apr 14, 2023

Yes, I wondered that too. And wonder if in the future they change their export if the import stops working...

@mickaelistria
Copy link
Contributor

And wonder if in the future they change their export if the import stops working...

That's exactly what the JUnit maintainers want to express with this flag: the package is exported for convenience but not as a public API. It's exactly like x-internal:=true except it has some advantages: it's OSGi standard and, more importantly, it makes that consumers are mandated to add the flag on Import-Package as a way to state "I understand the risk" so they cannot complain later if there is a breakage.

@merks
Copy link
Contributor

merks commented Apr 14, 2023

I was more concerned with what happens if they removed this flag in the future would the import stop resolving because the flag has gone missing?

@mickaelistria
Copy link
Contributor

You can try adding a foo=bar property to any existing Import-Package and see what happens. I think it should resolve to the package anyway as property are "decorations" to Import-Package, they're not constraint. The Export-Package defines whether a property is a constraint (via the mandatory directive). If there is no constraint worth checking, properties on Import-Package don't matter.

@HannesWell
Copy link
Member

Note: the package export is not a "usual" one one would expect, it looks like:

org.junit.vintage.engine;version="5.9.2";status=INTERNAL;mandatory:=status

Thanks Andrey for pointing that out. I can only confirm what Mickael has explained, that this indeed the reason for the problem.

To add some reference for those interested, the OSGi 8.0 Spec section 3.7.8 Mandatory Attributes specifies it:

There are two types of attributes: mandatory and optional. Mandatory attributes must be specified
in the import definition to match. Optional attributes are ignored when they are not referenced 
by the importer. Attributes are optional by default.

The exporter can specify mandatory attributes with the mandatory directive in the export definition.
This directive contains a comma-separated list of attribute names that must be specified by the 
importer to match. 

You can try adding a foo=bar property to any existing Import-Package and see what happens. I think it should resolve to the package anyway as property are "decorations" to Import-Package, they're not constraint. The Export-Package defines whether a property is a constraint (via the mandatory directive). If there is no constraint worth checking, properties on Import-Package don't matter.

That's not correct as OSGi 8.0 Spec section 3.7.7 Attribute Matching specifies:

In order for an import definition to be resolved to an export definition, the values of the attributes
specified by the import definition must match the values of the attributes of the export definition. 
By default, a match is not prevented if the export definition contains attributes that do not occur in 
the import definition. The mandatory directive in the export definition can reverse this by listing all 
attributes that the Framework must match in the import definition.

And if one tries what you have suggested, one gets a corresponding errors.
So Ed's concern is right and if the JUnit devs would decide one day to remove the attribute
Import-Package: org.junit.vintage.engine;status=INTERNAL will not resolve anymore.

What you could do is is adding two package-imports one with and one without the extra attribute and mark both as optional (by appending the usual ;resolution:=optional directive). But this has the drawback that the package is not pulled in with the plugin greedily. Therefore it is not ensured that the package is present. Furthermore there could be interesting results if that package is present from different version with and without that attribute.

Therefore I would just assume/hope that the JUnit devs added the attributes for a reason and will not just make those packages part of their 'regular API' in the future without renaming the package or a major version bump.

Btw. since there was also the question about = vs :=.

According to OSGi 8 section 1.3.2 General Syntax Definitions, = defines an attribute and := defines a directive.
And section 3.2.4 Common Header Syntax states:

A parameter can be either a directive or an attribute. A directive is an instruction that has some implied semantics for the Framework. An attribute is used for matching and comparison purposes.

@HannesWell : I believe you were involved in some PDE dependency management refactorings: could you please take a look?

Yes I can have a look this evening.

And sorry for the later reply but I was busy with other projects last night.

@cdietrich
Copy link
Contributor Author

@HannesWell should the add import dialog respect this behaviour?

@HannesWell
Copy link
Member

@HannesWell should the add import dialog respect this behaviour?

Probably yes. But it is a bit difficult since it is of course a valid choice to not add the extra attribute. There could be another provider of that package without any attribute. So just adding the attributes by default is not always right.
There should at least be a better error message or information for the user that the reason for the resolution error is a missing mandatory attribute and PDE could provide a quick-fix to add them.
On the other hand the package dialog could also highlight mandatory attributes and if you select a package exported with them could add them. But in case of different providers with different (mandatory) attributes they should all be listed with their different attributes.
So the question is what's more reliable and better to handle by a user and maybe simpler to implement?

@cdietrich
Copy link
Contributor Author

maybe a better error message (or a quickfix) would be helpful. but i have no idea how easy that can be done

@merks
Copy link
Contributor

merks commented Apr 14, 2023

@HannesWell @mickaelistria

Thanks both of you for the detailed information!

@HannesWell
Copy link
Member

maybe a better error message (or a quickfix) would be helpful. but i have no idea how easy that can be done

Created #578. I can work on it when time permits, but if anybody else wants to do it it is of course welcome. :)

@laeubi
Copy link
Contributor

laeubi commented Apr 16, 2023

So the question is what's more reliable and better to handle by a user and maybe simpler to implement?

Well the main problem is, that actually one should neither import the package nor require the bundle, this is currently a limitation of PDE and much better solved by JDT with the JUnit Classpathcontainer (that is suppressed by PDE...)

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

No branches or pull requests

7 participants