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

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

Closed
kwin opened this issue Oct 29, 2022 · 16 comments
Closed

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

kwin opened this issue Oct 29, 2022 · 16 comments
Assignees
Milestone

Comments

@kwin
Copy link
Contributor

kwin commented Oct 29, 2022

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 eclipse-sisu/sisu.plexus#28.

@cstamas
Copy link
Member

cstamas commented Oct 29, 2022

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
Copy link
Contributor

mcculls commented Oct 29, 2022

OK, will remove this packaging type for the next release

@kwin
Copy link
Contributor Author

kwin commented Oct 29, 2022

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.

@mcculls mcculls self-assigned this Nov 13, 2022
@lppedd
Copy link

lppedd commented Dec 3, 2022

@cstamas have a look at JetBrains/intellij-community#2127
Thoughts are always welcomed.

@cstamas
Copy link
Member

cstamas commented Dec 3, 2022

@laeubi ping ^

@lppedd
Copy link

lppedd commented Dec 3, 2022

@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
Copy link
Member

cstamas commented Dec 3, 2022

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

@lppedd
Copy link

lppedd commented Dec 3, 2022

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

@laeubi
Copy link
Contributor

laeubi commented Dec 3, 2022

@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
Copy link
Contributor

laeubi commented Dec 3, 2022

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
Copy link
Member

cstamas commented Dec 3, 2022

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. 😄 (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
Copy link
Contributor

laeubi commented Dec 3, 2022

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
Copy link
Contributor Author

kwin commented Dec 3, 2022

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
Copy link
Contributor

laeubi commented Dec 3, 2022

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
Copy link
Member

cstamas commented Feb 15, 2023

Isn't this done in a67f3ac ?

@mcculls
Copy link
Contributor

mcculls commented Feb 19, 2023

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

@mcculls mcculls closed this as completed Feb 19, 2023
@kwin kwin added this to the 1.0.0 milestone Feb 26, 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

No branches or pull requests

5 participants