-
Notifications
You must be signed in to change notification settings - Fork 0
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
How to contribute artifacts directly? #4
Comments
Some investigation is required... It fails like this:
I'd expect that to come from this which is present.
I'll investigate tomorrow. The stuff still lives git.eclipse.org... https://git.eclipse.org/c/oomph/org.eclipse.oomph.incubator.git/plain/OomphIncubator.setup |
Guice 5.1.0 indeed still requires Guava There was even the suggestion to remove the need for guava from guice, but that was also not answered: |
Yes, it needs this old thing:
Then it misses this:
And then I don't think there is a maven artifact that is also an OSGi library for this... What a mess.... |
I hope that is available in https://mvnrepository.com/artifact/com.google.errorprone/error_prone_annotations But yes that's all not so nice... Maybe you could also try to include compile dependencies in the Maven Location automatically? Then it should be simpler to set up and since only the required deps are in included in the repo we cannot avoid it anyway. |
These are not OSGi bundles: https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotation/2.9.0/ In general, I want to avoid using implicit dependencies because I want to know explicitly what is being pulled in and what version is being pulled in so that I can check those versions and report on them. I could generate bundle information for those, but that's no longer a direct-from-maven dependency where we simply reuse the jar (maybe even keep the PGP signature). It's pretty much what Orbit does with its BND recipes to repackage the jars. It seems to me that if we're not careful, everyone will happily repackage such things slightly differently, pulling in transitive dependencies without actually knowing what those are, producing multiple different artifacts with the same artifact key. That appears to be what this is doing: There's also an aopalliance dependency, which is not an OSGi bundle, but that a least is available in Orbit. It seems the m2e resolution is only considering the explicit pom dependencies: https://repo1.maven.org/maven2/com/google/inject/guice/5.1.0/guice-5.1.0.pom So it doesn't care that the package requirements of the OSGi bundle don't resolve:
That's going to be a problem when you try to actually use the bundle though isn't it? |
Oh man, that's really a mess.
Yes, that's understandable. But from my experience not every Maven dependency pulled into the TP by a Maven-TargetLocation is eventually required in an OSGi runtime. Especially Annotations are sometimes not needed. So altough they might be in the TP, they don't end up in a product or repo since P2 then only considers OSGi metadata and not maven-dependencies.
I agree that its bad, but I'm a bit hopeless that it would work without. As a side note, what do you think about migrating the BND-recipes in Orbit to use maven targets with explict BND-instructions that generated the OSGi metadata? That would also be an opportunity to sort out what's already available as OSGi-compatible jar on Maven-Central.
Yes, in such cases we will probably need a central repo forever that manages how such dependencies are OSGi-ified.
Yes the target-location can only pull in all (transitive) Maven dependencies if configured accordingly. Since that Target-location only knows about maven, that's the only thing it can do.
That's right and that's indeed likely a problem. Funny enough that the Maven dependency to errorprone.annotations only comes transitively through guava, which does not require it at runtime (at least according to its OSGi metadata). I looked a bit into the guice code and wonder if the |
I think it's very interesting to consider using the m2e target location for simple BND recipes and I'd like to explore that further, time permitting. But that time is not available until after next week at the earliest... |
I just created an issue to discuss that in Orbit: eclipse-orbit/orbit#8 |
I got the first thing, error_prone_annotations, working:
But the guava-base dependency doesn't work. Maybe because it's kind of a weird version number? The error is
That definitely exists though: https://repo1.maven.org/maven2/com/google/guava/guava-base/r03/ I also wonder about the details of the BND instructions. E.g., does it make sense to consider all package imports optional? I have my doubts about that. In general, will each artifact need specialized instructions? I kind of suspect so because r30 is a pretty strange version number. The generated manifest has quite some "noise" in it such that repeated builds will not produce the same manifest:
That's not so nice... |
That's right that this exists, but it seems to be an artifact from 2010: https://mvnrepository.com/artifact/com.google.guava/guava-base/r03
No we probably don't want to make all imports optional. This is just chosen as default to make it as flexible as possible and working as many cases as possible (e.g. often compile-time only deps are also listed) assuming that the required dependencies are actually specified in the pom. But since in general the BND-instructions are just a fallback it is totally fine to modify them to a more reasonable value for real world use. It of course depends on the exact metadata, but for what I have encountered so far e.g. with using wildcards smartly one set of instructions was sufficient.
You can use the -noextraheaders instruction to remove them. If that is not sufficient you can explicitly remove headers using the -removeheaders instruction (and list the headers to remove afterwards. The BND-lib references and incides can often be helpful:
To sum this up, I would use a location like the following. I tried to use the latest version possible.
|
Btw. I noticed that changed instructions are not always taken into account by m2e. I'll look into that tomorrow. |
That's helpful information! I forgot it just required an older version of guava for the "base" package, which is already a very annoying problem when trying to eliminate duplicates, and especially so for guava which tends to be visible in APIs... Going further, this does seem to be a bit of a rat hole with this failure trying what you showed:
With this failure without that dependency:
Note the package version range above which makes me wonder does this ancient really provide that package version? https://repo1.maven.org/maven2/javax/inject/javax.inject/1/ Given it's not an OSGi bundle, we can't really say can we. I will investigate further. |
Regarding this failure, the version range is probably coming due the presence of The good news is that in the 5.1.1-SNAPSHOT, since google/guice#1173 (which was just merged tonight) version range from the Furthermore with this change Import-Package header values were changed from version 5.1.0
to
The missing upper bound is not great in SemVer terms but at least allows to use Guice with a new major version of Guava, which comes relatively often but often only breaks not relevant parts. |
This issue suggests that this "findbugs" thing also leads to problems: eclipse-platform/eclipse.platform.ui#727 (comment) Even just updating what I have I hit this new problem:
Looking in the jar, I get the sense that this jar itself should be providing this capability: Is there something wrong with the new 1.4.7 version? |
The best thing would probably be to completely remove the import of javax.annotation from guice as suggested in the linked guice PR. I'll create a PR there. Alternatively proper version ranges could also help (jsr305 has Version 3 and the other one Version 1.x).
Ah, damn. I should have made them optional in qos-ch/logback#639 I'll provide a fix for logback this evening. |
Just created
Created |
@merks both PRs where merged. For Logback I'm not aware of any snapshot repo. |
I added it to the target and it builds, but it silently not actually included in the result. In fact, if I added it without -SNAPSHOT I see this in the log:
But no failure messages, and the build completes as if this dependency were not present at all. The PDE shows it as a problem though: Also for this: So I guess there is nothing I can test yet and I guess there is a bit of a problem in Tycho not reporting errors for missing dependencies... |
@merks Would it make sense to include the fix for Findbugs here? There is a lot more code out there which depends on this weird package. :( |
Ok, forget about the idea. It doesn't seem to work. The PDE Maven support is only generating OSGi metadata when it's missing. It's not updating/patching/manipulating existing (wrong) metadata. |
Yes, a repair needs a new publish. 😞 |
That's right. The possibility to generate a OSGi compliant manifest is intended as fallback and not to alter existing metadata. In general the goal of the Maven targets is to consumer original Maven artifacts, to not have diverging jars. Modifying the manifest of course modifies the jar. Without OSGi metadata there is no other ways than altering the jar afterwards, but in general the better way is always to contribute OSGi metadata upstream. IIRC we already had a discussion about that in the m2e issue tracker, but on a quick search I didn't found it. And in case of jsr-305 the metadata are right, it is just unfortunate that jsr-305 and javax.annotation spec share the same package name, Regarding JSR-305, it looks like its use is reduced e.g. the upcoming Guice 6.0 will not have any reference to |
I suppose technically one can wrap an unaltered jar in an OSGi bundle. But there’s not really an advantage to that! |
@merks sorry for the delay I now assembled a self-contained working target that includes
If you want to leverage transitive dependencies it can be trimmed to:
The main problem was that All other exclusions are actually/probably not necessary because they are not required by OSGi metadata. So even though they might end up as generated/wrapped bundles in the TP, they will not make it in any p2-repo unless they are explicitly included in a feature. At the moment this generates/wrapps an OSGi artifact for Do you want me to contribute that somewhere, to be ready when Guice 6 and 7 are released? |
It is very likely from the documentation that the errorprone annotations are only class retention annotations and therefore are not needed at all at runtime. |
Unfortunately they really do have retention policy runtime. :/ And I have to admit that I don't know what happens if one enoumerates over the annotations of a class and the annotation class cannot be loaded? Is that annotation just skipped, does the whole operation throw a Exception? |
Missing annotation are skipped, i.e. as if they were not applied/set. |
BTW, one thing that makes me a bit "paranoid" about wrapping is that if one makes a mistake in the recipe, or needs to update the recipe, one has to be concerned about creating a new IU/artifact-key with a higher version number. So these for example which are published here: https://download.eclipse.org/oomph/simrel-maven-bnd/nightly/N202305021250/index.html are really hard to fix if they are broken. My recipe didn't include One could of course do something in the recipe about the version.
But this recipe applies for all bundles in the group/feature, not per bundle, so if one had to fix one bundle one would have to split it out... With the Orbit approach, the qualifier comes from the commit of the corresponding project and if one changes the recipe, this commit time stamp changes. With this m2e target approach, there is nothing to correspond to that and it makes me concerned about how to manage this in case one doesn't get it exactly right the first time... |
Thanks for the link.
That's indeed a difficult situation. But regarding this specific case the Private-Package are not that bad. Headers unknown to the OSGi runtime are simply ignored and Private-Package is not used AFAIK, therefore this is just some noise in the Manifest but not too bad.
That's the only salutation I can think of to increment the version (append a manually specified suffix). Btw. from the MavenBND.target I would also remove the |
The PR has been merged just in time for the Guice 6/7 release, so for that we don't need error-prone anymore and therefore don't have to wrap it (the PR to add OSGi metadata to error-prone is merged too but not yet released). Nevertheless I'm trying to convince the Xtext devs, to use the Maven targets directly inhttps://github.com/eclipse-xtext/xtext/pull/2207. |
For eclipse-xtext/xtext#2056 I would like to add
com.google.inject:guice:5.1.0
to the simrel-orbit, but no other project that uses maven-targets contributes it.Can add it directly here? My first guess was to add it to the original.target in the suplement folder, but I'm not sure about that.
Can I just create a PR in this repo to add it?
The text was updated successfully, but these errors were encountered: