eclipse-sisu / sisu-project

Sisu Inject
https://eclipse.dev/sisu/
Eclipse Public License 2.0
18 stars 15 forks source link

Get rid of Tycho packaging "eclipse-plugin" #62

Closed kwin closed 1 year ago

kwin commented 1 year ago

Instead of leveraging the tycho packaging "eclipse-plugin" one should leverage packaging "jar" with https://github.com/bndtools/bnd/blob/master/maven/bnd-maven-plugin/README.md. This allows to get rid of a manually maintained MANIFEST.MF and properly supporting package versions via https://docs.osgi.org/javadoc/r6/annotation/org/osgi/annotation/versioning/Version.html.

This is supported to be referenced even from inside Eclipse with more recent Tycho/PDE versions via via https://xn--lubisoft-0za.gmbh/en/articles/using-maven-artifacts-in-pde-rcp-and-tycho-builds/. Main consumers of Sisu (IMHO Maven) are however outside of OSGi world anyways.

Compare with https://github.com/eclipse/sisu.plexus/issues/28.

cstamas commented 1 year ago

This, +1000. This essential project cannot be used in any other IDE than Eclipse for this very same reason (still, even 2022-09 Eclipse cannot open it without issues). Just stay away from it.

mcculls commented 1 year ago

OK, will remove this packaging type for the next release

kwin commented 1 year ago

I think at the same time the unit tests should be moved to the main module and the p2 site should be removed. Publishing Sisu to Maven Central only is enough given that PDE can consume Maven artifacts.

lppedd commented 1 year ago

@cstamas have a look at https://github.com/JetBrains/intellij-community/pull/2127 Thoughts are always welcomed.

cstamas commented 1 year ago

@laeubi ping ^

lppedd commented 1 year ago

@cstamas oh he knows, I've debated a lot about that 😆. That PR actually works, but obviously it must be extracted to a plugin and I'm literally submerged of work right now.

The real complexity lies in supporting multiple Maven versions, every one of them that carries a different set of dependencies. Plus the Embedder still doesn't offer enough flexibility for IDE support.

cstamas commented 1 year ago

Anyway, IMHO cleanest would be to make this project use "jar" packaging and be done with it... As issue reporter explained.

lppedd commented 1 year ago

@cstamas yeah that seems fine if it is easy to do. Just wanted to mention there is a way to use other IDEs.

laeubi commented 1 year ago

@laeubi ping ^

Any particular problem or issue some help is required? :-)

This, +1000. This essential project cannot be used in any other IDE than Eclipse for this very same reason (still, even 2022-09 Eclipse cannot open it without issues). Just stay away from it.

I don't know why the packaging type (!) prevents other IDEs from used/opening the project but actually this is then a bug in these IDEs and switching to "jar" does not change anything, because "eclipse-plugin" uses PDE meatadata and IntelliJ simply don't understand these meta-data, so one has to replace that first.

Anyways I don't see a reason why the proposed approach should not work in this context and as mentioned correctly an update-site should not be required anymore. Tycho is using sisu as well and it does that all with "plain" maven-jar packaging.

laeubi commented 1 year ago

A quick look only revealed

https://github.com/eclipse/sisu.inject/blob/master/org.eclipse.sisu.inject/plugin.xml

that seems should add special support for JDT, but I'm note sure if this is not already outdated (I think I have seen some conde in JDT to read the

https://github.com/eclipse/sisu.inject/blob/master/org.eclipse.sisu.inject/META-INF/services/javax.annotation.processing.Processor

instead, @iloveeclipse can you confirm this?

Anyways, if it is still required it could simply be moved to src/main/resources as-is.

cstamas commented 1 year ago

Problem is that this "bug" exists then in ALL major IDEs, since even latest Eclipse 2022-09 has issues opening this project. Hence, seems all IDEs out there are broken. :smile: (this was attempt for a joke, not beginning of a flame war)

Joking aside: removing custom packaging is even more so valid, since as you say, even Tycho is consuming sisu as "plain jar" packaging...

laeubi commented 1 year ago

Problem is that this "bug" exists then in ALL major IDEs, since even latest Eclipse 2022-09 has issues opening this project. Hence, seems all IDEs out there are broken. smile (this was attempt for a joke, not beginning of a flame war)

"issues" is just to generic I think, so what did you tried? Just from the code you need to import the projects and activate the target platform at least for Eclipse and everything should work (intelij /vscode do not have target support AFAIK), this project is really not any complex, but I have not worked with it (yet).

But thats nothing specific to the packaging type, as said Tycho only support this special PDE metadata to build with maven so it does not "do" anything here when you open the project in an IDE its the other way round but that fact is often mixed up.

Joking aside: removing custom packaging is even more so valid, since as you say, even Tycho is consuming sisu as "plain jar" packaging...

In the end its all a jar file (what actually is a zip file with a manifest as first entry), so beside the "site" (what could be replaced by a pure maven one if it is still desired) I really see no reason why it should not work.

kwin commented 1 year ago

For me this is mostly to leverage automatic MANIFEST generation and to attract more contributors (Maven knowledge is much more common than Tycho knowledge)

laeubi commented 1 year ago

Tycho is a Maven plugin and you don't need any special knowledge, you can edit any source-file and run the maven build without anything to "know" ;-)

As said before, Tycho just builds what PDE(!) Plugin produces as "IDE specific files" (you can use BND with Tycho as well if one likes ...) and I think that is not required here as actually nothing about PDE in this code repository it is actually plan Java with a little bit of additional Metadata for OSGi .... and then it doesn't matter much what maven-plugin one uses to package the stuff afterwards.

cstamas commented 1 year ago

Isn't this done in a67f3acbd8c75a6c4f54c70dcb4a86e97d041131 ?

mcculls commented 1 year ago

@cstamas yes - there will likely be further improvements wrt. packaging/versioning, but the original goal of moving to the simpler jar packaging is done.