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

Manage External Dependencies #103

Closed
merks opened this issue May 1, 2023 · 14 comments · Fixed by #106
Closed

Manage External Dependencies #103

merks opened this issue May 1, 2023 · 14 comments · Fixed by #106
Milestone

Comments

@merks
Copy link
Contributor

merks commented May 1, 2023

I'm concerned about how external dependencies are managed and in particular about any of the direct-from-maven dependencies that are generated. It's hard to tell which ones in the list are generated versus which ones are actually already well-formed OSGi artifacts:

<location includeDependencyDepth="infinite" includeDependencyScopes="compile,provided,runtime" includeSource="true" missingManifest="generate" type="Maven">
<dependencies>
<dependency>
<groupId>com.sun.activation</groupId>
<artifactId>jakarta.activation</artifactId>
<version>2.0.1</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>com.sun.xml.bind</groupId>
<artifactId>jaxb-impl</artifactId>
<version>2.3.3</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>jakarta.xml.bind</groupId>
<artifactId>jakarta.xml.bind-api</artifactId>
<version>2.3.3</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.apache.lucene</groupId>
<artifactId>lucene-analysis-common</artifactId>
<version>9.5.0</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.apache.lucene</groupId>
<artifactId>lucene-core</artifactId>
<version>9.5.0</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.apache.lucene</groupId>
<artifactId>lucene-queryparser</artifactId>
<version>9.5.0</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.eclipse.mylyn.github</groupId>
<artifactId>org.eclipse.egit.github.core</artifactId>
<version>6.1.0.202203080745-r</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.glassfish.hk2</groupId>
<artifactId>osgi-resource-locator</artifactId>
<version>2.4.0</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>4.8.1</version>
<type>jar</type>
</dependency>
</dependencies>
<repositories>
<repository>
<id>eclipse-egit</id>
<url>https://repo.eclipse.org/content/repositories/egit-releases/</url>
</repository>
</repositories>
<instructions><![CDATA[
Bundle-Name: Bundle derived from maven artifact ${mvnGroupId}:${mvnArtifactId}:${mvnVersion}
version: ${version_cleanup;${mvnVersion}}
Bundle-SymbolicName: ${mvnGroupId}.${mvnArtifactId}
Bundle-Version: ${version}
Import-Package: *;resolution:=optional
Export-Package: *;version="${version}";-noimport:=true
DynamicImport-Package: *
]]></instructions>
</location>

I really don't see how we can properly coordinate versions of libraries in SimRel when projects independently of each other generate wrappers for non OSGi-libraries. What's generated depends on the BND recipe:

<instructions><![CDATA[
Bundle-Name: Bundle derived from maven artifact ${mvnGroupId}:${mvnArtifactId}:${mvnVersion}
version: ${version_cleanup;${mvnVersion}}
Bundle-SymbolicName: ${mvnGroupId}.${mvnArtifactId}
Bundle-Version: ${version}
Import-Package: *;resolution:=optional
Export-Package: *;version="${version}";-noimport:=true
DynamicImport-Package: *
]]></instructions>
</location>

Each project may and likely will use a different recipe which appears likely to result in many different artifacts that use the same key such that they appear to be the same artifact from a p2 perspective but are actually different. That's not just a problem for SimRel but a problem in general when installing such content into any installation.

Here's an example:

https://github.com/eclipse/tm4e/blob/ac34c5f440f95e1046ecb69b1c3c5c8514cb4a86/target-platform/tm4e-target.target#L44-L52

Making all the package imports optional in the recipe is convenient but is highly likely to be wrong:

Import-Package:        *;resolution:=optional

In any case, these artifacts are not even being PGP signed as far as I can tell:

https://download.eclipse.org/oomph/archive/reports-extra/mylyn/download.eclipse.org/mylyn/stage/latest/index.html

image

It's also strange to see EGit's GitHub integration come from a Maven repository rather than more directly from EGit's downloads:

<repositories>
<repository>
<id>eclipse-egit</id>
<url>https://repo.eclipse.org/content/repositories/egit-releases/</url>
</repository>

https://download.eclipse.org/egit/github/

I feel that these generated direct-from-maven dependencies are opening up a new can of worms, and we've not even been able to fully manage the existing can of worms...

@ruspl-afed
Copy link
Contributor

Thank you for opening this issue, I'm also not happy with the way we consume libraries from maven, and tried to find a recommended way to do it but failed.

So, how should we properly declare an external maven dependency @merks ?

@merks
Copy link
Contributor Author

merks commented May 1, 2023

You should specify missingManifest="error" so that it fails for anything that is not available as an OSGi artifact:

https://github.com/eclipse-mylyn/org.eclipse.mylyn/blob/f8a6aa79cac4838c788101fd10f470971ba6bdce/org.eclipse.mylyn-target/mylyn-e4.26.target#LL69C81-L69C81

Anything that fails you should get from Orbit. I am current prototyping building a repository with such generated OSGi artifacts using an m2e target with BND recipes, but that is a work in progress that I will continue today...

Unless there is a compelling need for the absolute newest versions of these dependencies, I suggest you look here for those dependencies here:

https://download.eclipse.org/oomph/simrel-orbit/

If you get them from there, that also eliminate the need to enable PGP signing...

@laeubi
Copy link

laeubi commented May 1, 2023

Anything that fails you should get from Orbit.

I don't think that's a good suggestion, as orbit is always lacking behind and there is no compelling reason for a project to depend on orbit releases if the artifact is on maven, there might be some exceptions where it does not work, but in general the goal should be to get the OSGi metadata into the original library beside the following recommendations:

  1. Never (!) use require bundle for a third party dependency, but an import package with appropriate version range (either consumer or provider)
  2. Do not include third party dependencies in any feature.xml as dependencies will be pulled in anyways if used
  3. if wrapping is required, choose a custom prefix, e.g. Bundle-SymbolicName: mylyn.${mvnGroupId}.${mvnArtifactId} and review the resulting manifest if you publish them.

That way OSGi (and P2) can always substitute / reuse an alternative version of the artifact, in the worst-case there will be different wrapped ones with same content but that's nothing unusual/forbidden/dangerous, that's what OSGi was designed for!

@ruspl-afed
Copy link
Contributor

Unless there is a compelling need for the absolute newest versions of these dependencies

Ed, this is very different from what we discussed previously. Previously your message was: In the end, I think the process is very clear. Everyone use the latest version.

Let's imagine that Mylyn has a compelling need - what should we do then?

@laeubi
Copy link

laeubi commented May 1, 2023

Let's imagine that Mylyn has a compelling need - what should we do then?

Just see above, if you choose a different BSN (at best with a project prefix) there is absolutely no reason to not use a latest custom wrapped version (but also consider the last item in the list!)

@merks
Copy link
Contributor Author

merks commented May 1, 2023

@ruspl-afed

I tried to avoid drawing attention to what TM4E is doing because that looks like anarchy to me and is not what the Platform is doing. But now we're sliding down that slippery slope and will need to deal with that too. So we need the rule:

No generated Maven dependencies allowed.

What @laeubi suggests will mean every project creates its own key/version of generated IUs, so no collisions. But lots of duplicates, but at least they won't look like duplicates. If projects avoid bundle requirements then which variant ends up on the train seems indeterminate. It will be hard for projects to get those bundles into their repositories though, unless they include all requirements, in which case they will fill up their repo with the entire Eclipse platform and any other project on which they depended; maybe there is a solution for that too. If the generated package exports aren't versioned, one cannot control which version of you end up with, so that seems like a new form or anarchy.

Should we really start sliding down this slope without understanding what is at the end of that runway?

@laeubi
Copy link

laeubi commented May 1, 2023

So we need the rule:
No generated Maven dependencies allowed.

we absolutely don't, see above for a simple list of recommendations. This won't result in duplicates if all projects use the same version and semantic imports. And even if there are some duplicates this does not harm anything.

If the generated package exports aren't versioned, one cannot control which version of you end up with, so that seems like a new form or anarchy.

The automatic generated package exports always have a version.

@ruspl-afed
Copy link
Contributor

ruspl-afed commented May 1, 2023

as orbit is always lacking behind and there is no compelling reason for a project to depend on orbit releases

I think this is the key. What we need is the way to register approved 3rd party relatively quick, but still have it centralized to avoid variations and allow reuse.

And even if there are some duplicates this does not harm anything.

For me it is similar to situation where every project forks a class from Platform to its own package and then they are happy loaded together, since OSGi allows them to do so.

@merks
Copy link
Contributor Author

merks commented May 1, 2023

@laeubi

If I say we absolutely do need such a rule and you say we absolute don't, then that is the end of the discussion isn't it? Then there is no point is reasoning further because the lines are etched in the concrete. Then we will have to go to the supreme court of justice and get a ruling. It unfortunately feels futile to even have a discussion because only certain facts seem to matter. We saw the problem with gson duplicates last release cycle, yet you assert duplicates do not harm anything. At best they do no harm, other than bloat, but at worst they cause very hard to fix problems that we can of course blame on bad metadata, bad semantic versioning and other bad things that bad people do, but things that one one can actually solve as well.

@ruspl-afed

Yes, the idea is to produce update sites that can be quickly (in a matter of minutes) be produced and that are automatically updated to the latest version by a generator that looks at maven central.

You can see I've already been doing that:

https://git.eclipse.org/c/oomph/org.eclipse.oomph.incubator.git/log/
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/commits/master/eclipse.platform.releng.prereqs.sdk

But if centralized-except-for-maven is evil and decentralized generation and consumption is the only valid solution, then there is no discussion left to have, although a clear set of policies is need in either case...

ruspl-afed added a commit that referenced this issue May 1, 2023
Remove unnecessary maven egit repo

Signed-off-by: Alexander Fedorov <[email protected]>
ruspl-afed added a commit that referenced this issue May 1, 2023
Isolate EGit Maven repository

Signed-off-by: Alexander Fedorov <[email protected]>
ruspl-afed added a commit that referenced this issue May 1, 2023
[#103] Manage External Dependencies
@jonahgraham
Copy link
Contributor

we absolutely don't, see above for a simple list of recommendations. This won't result in duplicates if all projects use the same version and semantic imports. And even if there are some duplicates this does not harm anything.

We already know this is not true - otherwise I wouldn't have had to spend time respinning stuff to get rid of dup gson.

@ruspl-afed ruspl-afed added this to the 3.26.0 milestone May 1, 2023
@laeubi
Copy link

laeubi commented May 2, 2023

If I say we absolutely do need such a rule and you say we absolute don't, then that is the end of the discussion isn't it?

No I'm sure we will find enough 👍 in some way or another that there will be such a rule because people simply don't care do do things right and instead inventing new "rules" to make things even worse.

It unfortunately feels futile to even have a discussion because only certain facts seem to matter.

I presented some technical facts, but you argument with "another form of anarchy" (while this is actually a form of autonomy) so yes it seems that some specific feelings seem to matter much more than facts.

But if centralized-except-for-maven is evil and decentralized generation and consumption is the only valid solution, then there is no discussion left to have, although a clear set of policies is need in either case...

The only evil is to require rules to solve issues that won't occur without them, the simrel is so used to the "one-dependency-for-all" that projects simply don't care anymore about dependency management and correct usage of imports as they can easily offload it to others (especially you), the problem is that each of these "offloads" takes away autonomy from projects and increase the bus factor.

And to make it clear, it was not me that states " so we need the rule" that really feels like an absolute statement than a recommendation... So how can one respond to such as with another absolute statement?
Sad enough if you made such a statement is fine if other do so they are always promote the absolute evil ...

We already know this is not true - otherwise I wouldn't have had to spend time respinning stuff to get rid of dup gson.

Sorry I told the corresponding projects long before the issue happens that they use a wrong version range, but it was ignored so please don't twist the cause with the problem here, so these "issues" where not introduced by the fact that we have had duplicate versions, the duplicate versions where introduced because they where explicitly requested!

And think what, it was finally fixed (to some small extend) as I have recommended many times before and it just worked! So we don't need more rules, we need more recommendations in how to handle such situations properly beside from "I found something on SO/Google/outdated blog post/... that seem to fix my issue even though I don't understand why".

@merks
Copy link
Contributor Author

merks commented May 2, 2023

This does not feel like a discussion focused on a solution. More compelling would be to propose a well-formed vision of a better solution.

From what I understand the solution you've recommended is that each project use a project-unique name for their generated "wrappers", that the generated wrappers version all their package exports, and that no other bundle or feature ever reference those wrapper names. Does this fairly characterize what you see as the best/ideal/only solution?

Unfortunately no one has been doing that. In fact the default behavior (without a recipe) doesn't do that either. This appears to me a formula where 70 projects will actually and most definitely produce 70 variants of the "same library". Unfortunately, each variant of the 'same library" might use a different recipe, e.g., optional imports are the default, but probably not correct, so some might do that differently. In the end, the variants won't generally all be the same. Which one(s) end up aggregated and actually installed appears mostly non-deterministic as well. All that might well not cause any actual visible problems. But it feels like a significant risk to me and if it does cause problems those are not easily fixed.

Of course I don't that argue "do the wrapping in a central place" is 100% ideal either. You've noted some of the significant organizational drawbacks. I don't see any technical risks for SimRel, though perhaps I've overlooked them.

@laeubi
Copy link

laeubi commented May 2, 2023

This does not feel like a discussion focused on a solution

The problem is that a solution was proposed before the problem was understood, and that's a major problem in software development in general. So everything that starts with "we must do this" is obviously nothing focused in a finding solution because it already implies it... but that might be a personal feeling of me.

From what I understand the solution you've recommended is that each project use a project-unique name for their generated "wrappers", that the generated wrappers version all their package exports, and that no other bundle or feature ever reference those wrapper names. Does this fairly characterize what you see as the best/ideal/only solution?

It was claimed that there is no recommendation, so I tried to give some general advice how such situation (purely from a technical point of view) are handled in a flexible way. Whether or not this is assumed "best" is (for me) out of scope, because this depends on so many other factors.

At least it solves what was implied as a problem here as "how we can properly coordinate versions of libraries in SimRel when projects independently of each other generate wrappers for non OSGi-libraries" ... maybe is misunderstood the problem, or there are other concerns not addressed, this is not clear to me at this point.

So to summarize, I think this is a very good approach, and it would solve the problem, where the details might wary (e.g. depending on the deployment strategies used, a "product" has different demands to a "plugin" or a "library" type)

Unfortunately no one has been doing that.

I'm doing that from day 1 where this feature was added to Tycho+m2e and this is already quite a long time now. But the feature was designed to give liberty to their users to use it in a different way, so it is not the only way.

In fact the default behavior (without a recipe) doesn't do that either.

The defaults are the smallest possible valuable increment, as every default it can work for many cases but fail for others. nerveless you can insert any instructions here including what often is refereed to as a "receipe", there is literally no restriction!

This appears to me a formula where 70 projects will actually and most definitely produce 70 variants of the "same library".

First you propose that each project will use all the dependencies what is just not the case and as mentioned before, if it is very common to use such a lib, it is much more valuable to add such metadata in the library directly, wrapping was never meant as a general purpose solution, it is some kind of band-aid for libs you want to prototype, or no one will produce new release or alike, nerveless you can of course use it for other ways. and if you combine this with the Target-Reference-Location you can even share this among as many projects as you like.

Unfortunately, each variant of the 'same library" might use a different recipe, e.g., optional imports are the default, but probably not correct, so some might do that differently.

The point is that this literally does not matter, as such a wrapping never ever (well at least if you want to save you from trouble), should become part of your bundle-contract, you simply do import-package with proper version range and then your result will not depend on that (probably wrong shaped) wrapping!

So if I build something (like the simrel), I probably even keep a list of correctly and audited wrappings for a certain lib, or some time later there will be a new release of that lib that already carry the data, it simply do not matter for the artifact that use this for the sole purpose of compile and development from a different source!

Which one(s) end up aggregated and actually installed appears mostly non-deterministic as well.

Software usually always is deterministic. So if there is something that behaves that way it might be worth to fix that.

I don't see any technical risks for SimRel, though perhaps I've overlooked them.

The problem is, that there is already a "rule" that one should not depend on content from "the simrel" but if I remember correctly we have already had some issues because people simply do, especially we had dependencies from mylin / old orbits releases specifically see:

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/b5b94800896102d044d232165e8f72e2aac4f5da/eclipse.platform.releng.prereqs.sdk/eclipse-sdk-prereqs.target#L75-L79

also we have happens-before relationship already here:
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/b5b94800896102d044d232165e8f72e2aac4f5da/eclipse.platform.releng.prereqs.sdk/eclipse-sdk-prereqs.target#L49

So from a project point of view (even though maybe convenient) there is a high risk of unnoticed dependencies, even worse, once you have installed such service, you can hardly get rid of it, so from now on we:

  1. Have to maintain Orbit release
  2. Even older releases because some (accidentally) still rely on it
  3. Soon the "new central wrapping service" that will let people think it might be easier to use that service instead of helping projects maintain OSGi meta-data at first place.

@merks
Copy link
Contributor Author

merks commented May 2, 2023

Let’s call the new repo for generated artifacts “audited wrappings” and let’s hope one day soon we can tear off the band aides to find it empty.

SimRel/EPP really does make products from dozens of projects and the people doing that do have knowledge and experience with the problem history associated with that activity such that we might be assumed to have some understanding.

@wimjongman wimjongman added this to the 3.26.0 milestone May 5, 2023
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

Successfully merging a pull request may close this issue.

5 participants