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

A .target location to "Include dependencies from pom.xml" #418

Closed
mickaelistria opened this issue Dec 2, 2021 · 21 comments
Closed

A .target location to "Include dependencies from pom.xml" #418

mickaelistria opened this issue Dec 2, 2021 · 21 comments

Comments

@mickaelistria
Copy link
Contributor

As a user of Maven dependencies (OSGi bundles from Central) in a Tycho build
I'd like to have dependabot capable of editing Maven dependencies. This is already possible -and happening- if dependencies are defined in pom.xml (using target-platform-configuration/pomDependencies=consider|wrap); however it's not happening for .target files .
However, .target files are often important in the IDE for developers and contributors.

So I want dependabot working (updating deps) and .target working (provisioning deps in IDE and Tycho).
1 solution could be that dependabot adds support for .target files; but that's not a realistic thing to wish for.
Another possibility, which also brings PDE closer to Maven, would be to have in the .target a new target location or a customization of current Maven location "Include dependencies from pom.xml". Such a target location would get dependencies from the module/folder that holds the .target file (honoring parent and hierarchy), and populate the target-platform in PDE with those dependencies just like if they were directly defined in the .target.
This would cover the needs: dependabot will update deps from pom.xml, and those updates would become available in PDE when refreshing the .target.

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

One problem is that target files are not bound to a project/pom.xml in general, actually a target is not necessarily a file at all. Beside of that dependabot still will only be able to detect maven dependencies while other dependencies (e.g. p2 ones) are relevant as well.

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

Another issue is that m2e currently do not understand polyglot/pomless builds, also configuring automatic wrapping and alike would be problematic as it either requires that m2e parses tycho configurations or we invent another way to configure this with other approaches (e.g. magic properties).

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

1 solution could be that dependabot adds support for .target files; but that's not a realistic thing to wish for.

Why? Dependabot seems to be open source and for the sake of it it would be enough to parse the .target file as a regular xml file and search for maven like <dependency> tags... dosen't sounds that unrealistic to me.

@mickaelistria
Copy link
Contributor Author

One problem is that target files are not bound to a project/pom.xml in general, actually a target is not necessarily a file at all.

Sure, however I don't think this limitation makes the proposal less interesting. Usually, there is a parent pom that defines which .target to use, and a target-platform child module of this parent. Maven dependencies would be added as dependencies on the root pom, in the same file where .target is defined.
For Platform, mockito and other would be listed in the parent pom instead of being listed in the target, and we could have dependabot and license-check processing them.

Another issue is that m2e currently do not understand polyglot/pomless builds

Also another limitation that doesn't really matter here. As mentioned in the initial comment, in PDE the target-platform would be provisioned from the content of the .target file, and -if location enables it- the dependencies listed in the pom or its parent.
Overall, if adding a pom is necessary to enable dependabot and license-check automatically for each contribution, it would be worth it. At least for Platform, where this would be useful, there is a pom file for the .target file.

Why? Dependabot seems to be open source and for the sake of it it would be enough to parse the .target file as a regular xml file and search for maven like tags... dosen't sounds that unrealistic to me.

I unfortunately think it's a bit naive to assume that because a project is OSS, it will be open to including support (and associated maintenance effort) for a file format that's used by about 0.01% of its target audience. Moreover, the Eclipse community doesn't have much influence on releases and so on for dependabot. To me, this is more likely to fail than allowing m2e to instruct PDE to extend the .target resolution with the dependencies found in the pom hierarchy of this .target.

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

it will be open to including support (and associated maintenance effort)

when you never try it it won't happen of course ... it was often state we should not add support to tycho/m2e/whatever/... to fix problems/limitations of upstream/depending projects so why this rule does not apply here?

a file format that's used by about 0.01%

Maye its then time to consider dropping PDE and moving to maven-first based approaches like bnd-maven or felix-maven plugin instead. Given the discussions about maintenance burden it seems a bit ridiculous to add a whole bunch of restrictions to project setup and build tools just to use an external tool ... and maintaining dependencies trough pom.xml was never a goal for platform it was even argued that why people ever wanted to add maven items to their target ;-)

in PDE the target-platform would be provisioned from the content of the .target file

That is not always true. the .target file is just a serialization format but not the only source. Just think about files external to the workspace (can m2e handle foreign maven poms?) or the recently added URI-referenced support.
In case of maven coordinates the target file content could even be consumed from the maven repository itself, so what should then be the reference?

Also consider a child that references the target but don't use the same parent file, how should this work?

Essentially that's a similar discussion like with the "what additional repositories should be used" and can't be solved in a consistent way I think. If we really should consider this it would mostly make sense to reference the pom file instead of rely on the context (the default could be of course pom.xml in the current folder).

At least for Platform, where this would be useful, there is a pom file for the .target file

I'm really a bit concerned adding a feature that is completely driven by a specific setup (platform) and bound to a specific external tool that as you have stated

Eclipse community doesn't have much influence on releases and so on

So either we should participate in this tool and get some influence on its feature set or not depend on it too much here ...

If this is a real requirement I also think this would then better handled by a m2e-tycho-plugin that simply adds "considered" maven deps as M2_REPO classpathvariables that would work without introducing new target locations that are hard to get consistent.

@mickaelistria
Copy link
Contributor Author

when you never try it it won't happen of course ... it was often state we should not add support to tycho/m2e/whatever/... to fix problems/limitations of upstream/depending projects so why this rule does not apply here?

Feel free to try, but I wouldn't waste my time. Dependabot can easily challenge that the issue is that the .target format is alien to the outside world, so it's itself the upstream/root issue.

maintaining dependencies trough pom.xml was never a goal for platform it was even argued that why people ever wanted to add maven items to their target

Times are changing. External Maven libs without Orbit are making their entrance in Platform, those can come with dependabot and auto license check; those capabilities are actually more interesting than the .target itself in the case of Platform.

[.target file] is not always true.

It's often enough true to make it interesting. Other cases you mention are indeed valid, but they're not so frequent as this basic ".target file" case.

If we really should consider this it would mostly make sense to reference the pom file instead of rely on the context

Referencing explicitly a pom file instead of relying on "the pom in the same folder" actually sounds like a good idea... and the good thing is that it still matches the issue title, so it's a valid solution to initial problem description.

I'm really a bit concerned adding a feature that is completely driven by a specific setup (platform) and bound to a specific external tool that as you have stated

I gave Platform as 1 example, but soon, every Eclipse project built with Tycho will use Maven dependencies in .target or in their pom. And because of dependabot+licensecheck, I bet deps in pom.xml are going to win. At least, it becomes clear that's what we'll start using in most projects I'm dealing with, because it saves lots of time.

So either we should participate in this tool and get some influence on its feature set or not depend on it too much here ...

dependabot+licensecheck are an extremely useful and powerful toolset; they already saved us (Alex and I at least) several hours of work in the last weeks and it clearly is worth capitalizing on it. However, influence is not free, and getting influence on dependabot will be much more expensive and less safe for a result that's probably not as powerful technically speaking, than continuing to bend our current practices to make the Eclispe development workflows closer to more standard ones (eg more Maven).

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

eg more Maven

But why not then drop target as a whole and use pomDependecies=consider? The only missing link is then a m2e-(lifecycle?)-plugin that configures/updates the project with appropriate M2_REPO classpath-entries, then there will be no ambiguities, no need to manage those in the target and so on.

For me maven-target locations have the advantage that they are not requiring a pom/explicit maven context, if I have one I could better use that maven tooling instead directly....

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

dependabot ... are an extremely useful ...

just in case you haven't noticed yet, you can select maven target locations and simply hit the Update button to get the latest version for a maven dep in the target.

@akurtakov
Copy link
Contributor

eg more Maven

But why not then drop target as a whole and use pomDependecies=consider? The only missing link is then a m2e-(lifecycle?)-plugin that configures/updates the project with appropriate M2_REPO classpath-entries, then there will be no ambiguities, no need to manage those in the target and so on.

This sounds like very good alternative that will bring us closer to maven style. Have you thought in details about such plugin?

@laeubi
Copy link
Member

laeubi commented Dec 2, 2021

I tried to get some support for such things into PDE about a few months ago but it was classified as "not important for platform" but maybe things changing faster now :-)

Actually if you run mvn eclipse:eclipse it generates classpath entries that reflect the maven dependencies from the pom as using M2_REPO classpath variables and then you can use them (but need to manually update if you change the pom afterwards).

So one could have a JavaProjectConfigurator that do this automatically for the target-platform-configuration when there is pomDependencies=consider

Then we need to convince the PDE accept classpath-entries that are bundles as valid items in run-configurations ... @HannesWell is working in the area of run-configs afaik...

@akurtakov
Copy link
Contributor

I tried to get some support for such things into PDE about a few months ago but it was classified as "not important for platform" but maybe things changing faster now :-)

Well, different contributors have different priorities. I am not aware or at least don't remember such discussions.

Actually if you run mvn eclipse:eclipse it generates classpath entries that reflect the maven dependencies from the pom as using M2_REPO classpath variables and then you can use them (but need to manually update if you change the pom afterwards).

So one could have a JavaProjectConfigurator that do this automatically for the target-platform-configuration when there is pomDependencies=consider

Then we need to convince the PDE accept classpath-entries that are bundles as valid items in run-configurations ... @HannesWell is working in the area of run-configs afaik...

So the main issue is ^^^.

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

So the main issue is ^^^.

Here we go: https://bugs.eclipse.org/bugs/show_bug.cgi?id=577605

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

@mickaelistria I wonder if this requirement could already be fulfilled with a little tweaking of the current maven-pde target location.

  1. Given we have a pom that declares all dependencies that are desired for the target (could be of course a parent pom but that's not necessary)
  2. this project is of packing-type pom and has some coordinates e.g. org.eclipse.platform : pom-dependencies : 1.0
  3. you can now reference this pom in the target as a location and all its dependencies will be pulled in as child's

The only 'missing-link' is that currently the m2e-pde does not support workspace resolution so you need to mvn install this pom first to use it in the target...

pom support is a bit basic in the current release but I have enhanced it with

so maybe this would be a valid path for platform-needs?

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

#280 (comment) shows a screenshot how a pom-dependency works in the target, one could choose to either only include direct or also transitive dependencies...

@mickaelistria
Copy link
Contributor Author

I wonder if this requirement could already be fulfilled with a little tweaking of the current maven-pde target location. [...]

I actually came to the same idea last night before falling asleep ;) I agree being able to reference a pom.xml and expanding it to transitively include all dependencies would cover the story quite nicely, better than with a separate Target Location.

The only 'missing-link' is that currently the m2e-pde does not support workspace resolution so you need to mvn install this pom first to use it in the target...

That would be acceptable; what matters more for Platform story at the moment is the Tycho part more than the PDE one. So even without workspace resolution, it would be a good solution I believe.
Do the pom dependency "expansion" in .target work for Tycho as well?

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

Do the pom dependency "expansion" in .target work for Tycho as well?

Not right now as I'd like to wait until its available in m2e

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

Sorry for confusion, the pom support was already released, anyways it is not yet implemented for tycho but on my todo list for the next weeks.

@mickaelistria
Copy link
Contributor Author

I'm closing it because the pom support in m2e covers the requirement.
Thanks a lot @laeubi for the discussion and the feature which I didn't yet have the opportunity to acknowledge!

@laeubi
Copy link
Member

laeubi commented Dec 3, 2021

The only 'missing-link' is that currently the m2e-pde does not support workspace resolution so you need to mvn install this pom first to use it in the target...

That would be acceptable

But for the matter, can someone give me a hint how project resolution works? Is there a class I can call with maven coordinates and get the project or something similar?

@mickaelistria
Copy link
Contributor Author

Is there a class I can call with maven coordinates and get the project or something similar?

MavenPlugin.getMavenProjectRegistry().getMavenProject() or
MavenPlugin.getMaven().getSession().getRepositorySession().getWorkspaceReader().findArtifact() maybe.

@laeubi
Copy link
Member

laeubi commented Dec 14, 2021

@mickaelistria @akurtakov I think everything is in place now for m2e + tycho to support this use-case would be cool if one of you could make a POC for the platform.case so we can check if anything is missing or not working as intended. Just not that currently latest SNAPSHOT for m2e+tycho is required to work with this.

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

3 participants