tycho icon indicating copy to clipboard operation
tycho copied to clipboard

Support JDKUsage.CLASSPATH as an alternative to SYSTEM/BREE

Open laeubi opened this issue 3 years ago • 17 comments

The classpath contains an entry of the JRE that is actually used by eclipse:

<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11">
</classpathentry>

there should be an option for tycho to use this.

Actually CLASSPATH should be the default and I would expect the following delegation model:

  1. the classpath is considered first
  2. a toolchain is searched that matches the given JRE
  3. if no toolchain is found and the current running one is used (equals SYSTEM)
  4. if the running one do not match, a warning is printed

laeubi avatar May 17 '21 06:05 laeubi

For compilation, .classpath may be considered first, internally in tycho-compiler-plugn. For dependency resolution, it has to be driven by OSGi, so MANIFEST.MF has to be used and .classpath must be ignored at that stage. Overall, I have the impression supporting JRE from .classpath would add a lot of extra complexity for no clear added value to balance it.

mickaelistria avatar May 17 '21 06:05 mickaelistria

Sure this is only for compilation, the complexity is actually much lower than the current BREE implementation about the effort taken to get a and could reuse most of its code (e.g. finding toolchains).

no clear added value

The value is simply that I can reuse classpath configuration and control a lot more what JRE is used.

Lets say I have a bundle that needs to be build against a special VM (e.g. gralvm) currently my only option is to compile ALL bundles with gralvm because I can't configure different JDKs for the same BREE id.

laeubi avatar May 17 '21 06:05 laeubi

I think we should remove <useJDK>BREE</useJDK> in favor of this feature in Tycho 3.0. It is just strange to setup and given that the same could better be expressed in .classpath we should simply use that.

Beside that BREE is no longer required/recommended by OSGi for modular VMs...

laeubi avatar Feb 04 '22 06:02 laeubi

Some projects don't like to commit their .classpath and would still need a way to express that kind of customization in the pom file.

mickaelistria avatar Feb 04 '22 08:02 mickaelistria

We probably don't like to support such projects anymore ;-)

Actually we already require a build.properties and as tycho is for building eclipse projects it makes sense to require .classpath for certain features as well as we require one for the compiler settings.

laeubi avatar Feb 04 '22 09:02 laeubi

We probably don't like to support such projects anymore ;-)

Why? After all those projects have nothing in .classpath that cannot be computed dynamically and just tweak their build (not the IDE behavior), so why should we just tell them they're wrong and we don't want to support them? Note that plugins like VSCode put .project and .classpath in invisible folders, out of the source tree, and people are really happy with it. It's technically pretty valid to not add dumb .classpath file that have nothing smart in them and let the IDE regenerate them on import. I don't think we should break those users.

mickaelistria avatar Feb 04 '22 09:02 mickaelistria

Why? After all those projects have nothing in .classpath that cannot be computed dynamically and just tweak their build (not the IDE behavior),

Tycho supports many aspects of .classpath already and tycho should behave as close as possible to the IDE to not have different outcomes between Tycho<->IDE

So if I configure the JRE already in .classpath why configure it in another (possibly diverging) tycho configuration property that only tries to guess what might be there from a property that is no longer required to be there since Java 9 support in OSGi?

so why should we just tell them they're wrong and we don't want to support them?

There are just a better way to express this now. Binding to the EE is just wrong as it is

a) optional since a while b) is a runtime requirement and not a compile requirement c) could contain multiple items where its unclear what to choose

Note that plugins like VSCode put .project and .classpath in invisible folders, out of the source tree, and people are really happy with it.

These people can still use VSCode and probably will never use Tycho :-) If Eclipse would support something like that any far away day in the future we would need to think about how to handle those cases.

It's technically pretty valid to not add dumb .classpath file that have nothing smart in them and let the IDE regenerate them on import.

Not all data could be reliable generated. they are not "dumb" but contain valuable information. I'd rather would drop build.properties because that are really dumb files , Some people even argue that MANIFEST.MF could be generated but we use them so why make a difference here?

I don't think we should break those users.

We won't break them as most users simply don't use this feature in the case you have described. There is only one integration test for this in tycho and it feels more like a workaround as a valid solution.

laeubi avatar Feb 04 '22 09:02 laeubi

Tycho supports many aspects of .classpath already and tycho should behave as close as possible to the IDE to not have different outcomes between Tycho<->IDE

That's 1 possibility, it doesn't make other ones invalid or incorrect.

Not all data could be reliable generated. they are not "dumb" but contain valuable information

They rarely do in the project I work with. Everything in them can be trivially regenerated by PDE according to MANIFEST.MF. So this files could even be derived or removed and generated and everything would still work.

I'd rather would drop build.properties because that are really dumb files ,

Go for it ;)

Some people even argue that MANIFEST.MF could be generated but we use them so why make a difference here?

Yet we didn't delete Tycho because PDE exists. We acknowledge that there is a use-case for MANIFEST-first and maintain it. I think we need to also acknowledge there is a use-case for pom-first over .classpath first and maintain it as well.

We won't break them as most users simply don't use this feature in the case you have described. There is only one integration test for this in tycho and it feels more like a workaround as a valid solution.

That would be a more compelling reason: if we audit the use-cases for this and fine none that seems to make sense, it could be OK to remove it; but saying "let's remove it because people should use .classpath-first anyway" is not a sufficient nor good reason.

mickaelistria avatar Feb 04 '22 09:02 mickaelistria

They rarely do in the project I work with. Everything in them can be trivially regenerated by PDE according to MANIFEST.MF

well simply delete the .classpath and import the project and you will see that it will completely fail.

It will only work for pom.xml configured projects and those can configure the jdk in the maven compiler settings no need for a taret confiuration.

So we have two cases:

  1. everything is defined in pom.xml and can be derived from it -> one won't need any eclipse 'dotfiles'
  2. one uses eclipse dotfiles and pom.xml only for configurations that tycho currently do not understand

what I'm aiming at is the second case as there the "pom" is the dumb file that could be generated by the pomless build-extension and I don't need to create it at all.

but saying "let's remove it because people should use .classpath-first anyway" is not a sufficient nor good reason.

I don't say people should use it the just should use it when the want use a specific jvm as this is already expressed in the .classpath in a much more reliable and consistent way, you really don't want to develop with a JDK11 and compile with JDK8 on the CI.

laeubi avatar Feb 04 '22 09:02 laeubi

well simply delete the .classpath and import the project and you will see that it will completely fail.

Without .classpath and .project, there is usually 0 issue. The "Import projects from folder" wizard will recognize the projects as being PDE and Maven, will enable the natures and generate a .classpath that works with those.

So we have two cases:

I see a 3rd case, which is the "ideal" one: one has only a MANIFEST.MF and both Eclipse IDE and Tycho are able to derive everything necessary for it. And this is almost already the case here. Those may set only 1 parent pom and have <useJDK>BREE</useJDK> set to get some consistency.

what I'm aiming at is the second case as there the "pom" is the dumb file that could be generated by the pomless build-extension and I don't need to create it at all.

That's fine, however we will never eliminate the cases of many bundles tweaking their build and for which pom.xml would still be present. So we just need to ensure things still work fine and as expected for those (numerous) cases. However, it's important to remind that the <useJDK>BREE</useJDK> can be set in the parent pom even when modules themselves are pom-less. So pomless doesn't currently imply people don't/can't use <useJDK>BREE</useJDK>.

I don't say people should use it the just should use it when the want use a specific jvm as this is already expressed in the .classpath in a much more reliable and consistent way, you really don't want to develop with a JDK11 and compile with JDK8 on the CI.

Which "useJDK" option do you have in mind? The one from tycho-compiler-plugin? The one from tycho-surefire-plugin? Both? Still, I'm not convinced that <useJDK>BREE</useJDK> is better expressed in .classpath as .classpath duplicates some information that can be derived from MANIFEST.MF (BREE field or osgi.ee requirement). Some people like that they change their MANIFEST and the build adapts, and may exclude .project/.classpath from their Git repo to avoid remediating those case of lack of synchronization between dev and build info. Those people may use <useJDK>BREE</useJDK> to enable that and can be happy with it as they just set things in 1 place instead of 2. There are such people, we shouldn't break them unless their is enough value to match their pain, or a better alternative that covers their need better (spoiler: adding a static .classpath file that duplicates the JDK info is not a better alternative).

mickaelistria avatar Feb 04 '22 10:02 mickaelistria

Without .classpath and .project, there is usually 0 issue. The "Import projects from folder" wizard will recognize the projects as being PDE and Maven, will enable the natures and generate a .classpath that works with those.

Only if you have a pom.xml

I see a 3rd case, which is the "ideal" one: one has only a MANIFEST.MF and both Eclipse IDE and Tycho are able to derive everything necessary for it.

That will only work for a 'flat' setup where there root folder is the source folder.

That's fine, however we will never eliminate the cases of many bundles tweaking their build and for which pom.xml

I'm building projects with hundred of plugins and no pom.xml necessary beside the usual root.pom all these pom.xml customization are manual to overcome current or past tycho limitations but are not mandatory. And if they are more an exception.

it's important to remind that the <useJDK>BREE</useJDK> can be set in the parent pom even when modules themselves are pom-less. So pomless doesn't currently imply people don't/can't use <useJDK>BREE</useJDK>.

pomless are meant to use eclipse config files to derive all neccesary data from it. Beside that I never have had any use for <useJDK>BREE</useJDK> if tycho would simply understand the classpath :-)

Especially for a pomless build I can't omit these files anyways.

is better expressed in .classpath as .classpath duplicates some information that can be derived from MANIFEST.MF (BREE field or osgi.ee requirement).

That is not true. BREE field or osgi.ee requirement are runtime constraints and have nothing to do with the JVM to compile with. And if you want to configure a custom JVM (what works in JDT) you are simply lost with tycho.

There are such people, we shouldn't break them

Sure if those people are fine with supporting those odd use-cases why not. Currently this simply complicates thins without any value and is even marked in tycho code for a long time to not belonging to the target configuration...

laeubi avatar Feb 04 '22 10:02 laeubi

Just for curiosity I searched in platform code for

  • <useJDK>BREE</useJDK> -> no match
  • <useJDK> -> no match

and in tycho code one reference in compiler-pluin test and one reference in interation tests...

laeubi avatar Feb 04 '22 10:02 laeubi

Only if you have a pom.xml

If you don't have a pom.xml, then it's configured as a plugin project and .project/.classpath are generated. That doesn't prevent to build with Maven/Tycho if some parent pom uses it.

That will only work for a 'flat' setup where there root folder is the source folder.

Wrong. The importers are capable of detecting the root source folders and configuring the .classpath accordingly, and correctly in most cases.

pomless are meant to use eclipse config files to derive all neccesary data from it.

Usually, only the MANIFEST.MF is enough. Most other things can usually be derived, even .classpath or build.properties.

I'm not against removing <useJDK>BREE</useJDK>, but I find a bit dangerous to quickly pretend it's bad and and claim "people should do X instead" where X is really not a trivial change and may not be functionally equivalent. I also find it dangerous to be build technical decision on a too opinionated ".classpath is better than pom.xml or MANIFEST.MF" while there is nothing technically obvious to back that opinion. See https://github.com/search?q=useJDK+BREE&type=code about some user stories for useJDK=BREE. We need to get a sense of what are those user-stories and evaluate whether those stories are really worth being broken.

mickaelistria avatar Feb 04 '22 10:02 mickaelistria

Just for curiosity I searched in platform code for

You missed https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/master/eclipse-platform-parent/pom.xml#L708 , and this bree-libs profile is set on all CI builds. But Platform does commit .classpath and duplicates the setting which is deduced from the MANIFEST.MF (most .classpath files are dumb ones derived from MANIFEST.MF), so relying on some .classpath here would still work; until we decide that after all, those .classpath can be removed and go for dynamically creating them in some secret folder, like VSCode does, so we don't have to maintain them any more.

mickaelistria avatar Feb 04 '22 10:02 mickaelistria

Classpath files are not generated. They are created as part of the project setup. and even if eclipse is capable of generation them "most of the time" if other metadata is there (build.properties(!)) this does not mean they are useless.

I never used VSCode but at least intelij idea also do it this way with the consequence that you hardly can have projects from different workspaces in one window.

Anyways keep the BREE hack then I just won't mind about it as I don't use VSCode and don't use Idea but Eclipse and simply don't want to use special configuration just for the sake of not keeping relevant files in the source repository.

laeubi avatar Feb 04 '22 10:02 laeubi

we don't have to maintain them

Eclipse 'maintain' them for me so I don't care and it does not add any overhead.

laeubi avatar Feb 04 '22 10:02 laeubi

You missed

that everything is cluttered :-D Anyways at least that setting would better be part of the tycho-compiler than the tycho-target platform as this is clearly a compiler directive.

Wrong. The importers are capable of detecting the root source folders and configuring the .classpath accordingly, and correctly in most cases.

Then I'm the one that always find the cases where it not works ;-) I simply do not want wasting time because some crippled setting in one of those files are accidentally got wrong... also m2e does need some special help because it does not detect always all source folders (see recent post on the m2e-dev list) and even if there are no sourcefolders not present misses adding the java nature and so on ... so I'm not very used to these "mostly automatic" things that are 80% guessing and 20% based on data present.

laeubi avatar Feb 04 '22 10:02 laeubi