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

API Tools failure #103

Closed
mickaelistria opened this issue Mar 14, 2023 · 23 comments · Fixed by #104
Closed

API Tools failure #103

mickaelistria opened this issue Mar 14, 2023 · 23 comments · Fixed by #104

Comments

@mickaelistria
Copy link
Contributor

Building locally from master, mvn verify -Pbuild-individual-bundles,api-check -pl org.eclipse.tips.ide fails with

[INFO] --- tycho-eclipserun-plugin:4.0.0-SNAPSHOT:eclipse-run (api-analysis) @ org.eclipse.tips.ide ---
[WARNING] Using JavaSE-19 to fulfill requested profile of JavaSE-17 this might lead to faulty dependency resolution, consider define a suitable JDK in the toolchains.xml
[WARNING] Using JavaSE-19 to fulfill requested profile of JavaSE-17 this might lead to faulty dependency resolution, consider define a suitable JDK in the toolchains.xml
[WARNING] Using JavaSE-19 to fulfill requested profile of JavaSE-17 this might lead to faulty dependency resolution, consider define a suitable JDK in the toolchains.xml
[WARNING] No toolchain was found in tycho-eclipserun-plugin for: JavaSE-17. Current Java runtime will be used
[INFO] Expected Eclipse log file: /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace/data/.metadata/.log
[INFO] Command line:
	[/usr/lib/jvm/java-19-openjdk-19.0.2.0.7-1.rolling.fc37.x86_64/bin/java, -Xmx2048M, -Dp2.RepositoryPreferences.retryOnSocketTimeout=true, -Dp2.RepositoryPreferences.connectionRetryCount=3, -Dp2.RepositoryPreferences.connectionMsRetryDelay=500, -Dorg.eclipse.ecf.provider.filetransfer.retrieve.readTimeout=10000, -jar, /home/mistria/.m2/repository/p2/osgi/bundle/org.eclipse.equinox.launcher/1.6.400.v20210924-0641/org.eclipse.equinox.launcher-1.6.400.v20210924-0641.jar, -install, /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace, -configuration, /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace/configuration, -data, /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace/data, -application, org.eclipse.pde.api.tools.apiAnalyzer, -project, /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide, -baseline, /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/target/org.eclipse.tips.ide-apiBaseline.target, -dependencyList, /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/target/dependencies-list.txt, -failOnError]
Some blocking (most likely link/compilation) errors are present:
[FATAL] File TipsStartupService.java at line 35: The import org.osgi.service.component cannot be resolved (location: /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/src/org/eclipse/tips/ide/internal/TipsStartupService.java)
[FATAL] File TipsStartupService.java at line 44: Component cannot be resolved to a type (location: /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/src/org/eclipse/tips/ide/internal/TipsStartupService.java)
Some blocking (most likely link/compilation) errors are present ^^^

I observed with a debugger running java -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=1044 -Xmx2048M -jar /home/mistria/.m2/repository/p2/osgi/bundle/org.eclipse.equinox.launcher/1.6.400.v20210924-0641/org.eclipse.equinox.launcher-1.6.400.v20210924-0641.jar -install /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace -configuration /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace/configuration -data /home/mistria/git/eclipse.platform.ua/apiAnalyzer-workspace/data -application org.eclipse.pde.api.tools.apiAnalyzer -project /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide -baseline /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/target/org.eclipse.tips.ide-apiBaseline.target -dependencyList /home/mistria/git/eclipse.platform.ua/org.eclipse.tips.ide/target/dependencies-list.txt -failOnError (the command generated by eclipse-run, with addition of debugging agent), and connected to the application, I can see that the target platform seems to be properly resolved, includes the org.osgi.service.component bundle, that the org.eclipse.tips.ide project is imported but I don't see org.osgi.service.component in its resolved classpath (while both Tycho and PDE in my IDE seem to add it to the classpath).

API Tools application creates a target-platform from the dependency-list file, and imports the project as source in the workspace, requests the build, then PDE starts working and the application returns filter API Tools problems. So it's mostly plain PDE, nothing fancy over a regular developer workspace. But devil hides in the details, and there is 1 detail here we're missing.
In the current state the issue is that: PDE adds org.osgi.service.component to the build path org org.eclipse.tips.ide in a regular workspace but doesn't add it when running the API Tools application.

@merks
Copy link
Contributor

merks commented Mar 14, 2023

So it tries to import org.osgi.service.component.annotations.Component but it doesn't import that package but it rather seems to rely on some bundle requirement that is exporting its imports:

image

All mention of that package seem require it optionally (except for the one bundle that exports it):

image

It would maybe/probably help add a package import like this (like org.eclipse.core.resources does):

org.osgi.service.component.annotations;version="[1.3.0,2.0.0)";resolution:=optional

@akurtakov
Copy link
Member

What bothers me is that compile works but not apitools . If the overall recomendation is to add optional import package, I would do that.

@laeubi
Copy link
Contributor

laeubi commented Mar 14, 2023

All mention of that package seem require it optionally (except for the one bundle that exports it):

Please search the source code and not only the manifests.

It would maybe/probably help add a package import like this (like org.eclipse.core.resources does):

Please don't do that! The annotations are only retained at the classlevel so they are not relevant at runtime.

If the overall recomendation is to add optional import package, I would do that.

it is NOT recommended to import these, it is actually discouraged!

What bothers me is that compile works but not apitools

Both Tycho and PDE add these to the compile classpath of a bundle specifically PDE has org.eclipse.pde.internal.core.annotations.OSGiAnnotationsClasspathContributor for this purpose.

@mickaelistria mickaelistria linked a pull request Mar 14, 2023 that will close this issue
@merks
Copy link
Contributor

merks commented Mar 14, 2023

I did search all sources. What should I find out from that search?

Many things are apparently doing not recommended things. What does that tell us? Maybe something newer was introduced to solve a problem that previously was solved that way?

Might it be the case that this API tools thing is running without OSGiAnnotationsClasspathContributor, which is relatively new? (I wonder how one generally deals with annotations needed at compile time but not runtime when special OSGi handling does not handle it?)

Any other theories or suggestions?

@mickaelistria
Copy link
Contributor Author

Submitted #104 as a possible solution.
I don't think PDE is wrong by not adding it in such case, or even in other cases. That's the "magic" with optional and transitive dependencies. Here the dependency is direct and non-optional for proper dev/build, so my PR explicits it in build.properties.

@laeubi
Copy link
Contributor

laeubi commented Mar 14, 2023

I did search all sources.

Your screenshot shows a search in manifest only

What should I find out from that search?

That not "All mention of that package seem require it optional", this is simply the an old hack to overcome the limitations of PDE but not the only one that was in place for this.

Many things are apparently doing not recommended things. What does that tell us?

That no one is eager to clean this up maybe...

Might it be the case that this API tools thing is running without OSGiAnnotationsClasspathContributor, which is relatively new?

That's what should actually be investigated here, instead we choose yet another workaround, but honestly I don't care enough to fight again the windmills ... I just can point out that this is wrong and that there are more modern alternatives for this that support OSGi best practices.

I wonder how one generally deals with annotations needed at compile time but not runtime when special OSGi handling does not handle it?

Generally one adds them as compile-time dependencies, maven user might choose provided scope to not let consumers inherit these, there are many strategies...

@mickaelistria
Copy link
Contributor Author

That's what should actually be investigated here, instead we choose yet another workaround, but honestly I don't care enough to fight again the windmills ... I just can point out that this is wrong and that there are more modern alternatives for this that support OSGi best practices.

If you're willing to investigate more and possibly revert the workaround in favor of a better solution, it is more than welcome. But in the meantime, proper maintenance of the repo is blocked by this issue, and we need to make progress anyway; we needed to get rid of the blocker quickly without decreasing quality, and the workaround does that job; nothing more.

@merks
Copy link
Contributor

merks commented Mar 14, 2023

When I did a full search, there were many matches:

image

Was there something specific I should have done with that? It seemed Java matches weren't interesting and that MANIFEST.MF would show me dependencies (imports and exports). In any case, it's really, really very easy for anyone to set up an IDE where anyone can do such searches.

I see these uses of additional.bundles in build.properties, including this newly added thing now:

image

The pragmatic problem (as @mickaelistria highlights) is that most folks have an immediate problem to solve and don't have the time to solve problems in the tools they are using to solve problems, i.e., solving metadata problems. It's unfortunate but reality often is...

I'm sorry you feel you are fighting windmills. I hope I am not a windmill. I do get the feeling that I'm being criticized merely for trying to help, though that's probably just me being over-sensitive...

Can you be more specific about what the correct PDE solution is solution is for adding a dependency on a package to provide access to development time annotation that is not actually needed at runtime? I.e., is this additional.bundles approach not correct or appropriate in the general case? (I understand that the OSGiAnnotationsClasspathContributor should kick in, but I don't think that's a general solution for arbitrary annotations.)

@laeubi
Copy link
Contributor

laeubi commented Mar 14, 2023

But in the meantime, proper maintenance of the repo is blocked by this issue

In this regard we said that we want to allow usage of Java 17, I can't remember that it was a requirement to forcefully require Java 17 under all circumstances... So "Move webapp to Java 17" is either misleading or not really a blocker, also the build now seems to use even Java 19 what might be another problem even though I have no idea how this can be related.

If you're willing to investigate more and possibly revert the workaround in favor of a better solution, it is more than welcome.

I'll see if I can find some time, but usually if there is a workaround in place there is little to no force to change anything if not even there is some moan why to change anything at all if everything is working...

Can you be more specific about what the correct PDE solution is solution is for adding a dependency on a package to provide access to development time annotation that is not actually needed at runtime?

As @mickaelistria already pointed out, everything works out-of-the-box for PDE (and Tycho), you can simply remove the import, and I think we already have some bundles (equinox for example) doing it that way without any issues, so there is some problem in the ant-based API tools somewhere. And as mentioned elsewhere, if no one has the time to fix/enhance the tool, we probably should throw it away as it will accumulate technical dept if no one cares about the tool itself.

additional.bundles approach not correct or appropriate in the general case?

additional.bundles is what PDE under the hood refers to as "Secondary Dependencies" and the UI presents as "Automatic Manged Dependencies", and their purpose is that you can add some bundles there and PDE automatically adds an import/require bundle if you run the bundle in the IDE... thats certainly not what one want here as it potentially modifies the manifest to use the import-package (now as a strict requirement) again.

I understand that the OSGiAnnotationsClasspathContributor should kick in, but I don't think that's a general solution for arbitrary annotations.

There is a feature in the target platform that seems to server the purpose but is is sadly broken:

eclipse-pde/eclipse.pde#451

my plan is to try to restore the functionality but have not yet deeper investigated the history of that.

@akurtakov
Copy link
Member

I've edited rather than comment sorry about that. Restoring it.

@akurtakov
Copy link
Member

So where is Java 19 used ?
When I have to do the unbreaking I move to Java 17 directly as it is allowed now and I don't plan to spend time doing only version bumps.

@mickaelistria
Copy link
Contributor Author

In this regard we said that we want to allow usage of Java 17

That's not the only thing the PMC has agreed IIRC, but there is also an agreement to drop support for older Java versions (in order to ease maintenance).

@laeubi
Copy link
Contributor

laeubi commented Mar 14, 2023

So where is Java 19 used ?

See above log output:
[WARNING] Using JavaSE-19 to fulfill requested profile of JavaSE-17

in order to ease maintenance

How has this eased the maintainance? :-)

Anyways if it has worked before but failed afterwards, something more seems to be changed.

@mickaelistria
Copy link
Contributor Author

How has this eased the maintainance? :-)

I'm pretty sure the same comments were made when Eclipse did drop Java 3 support, then Java 5, then Java 8... This is an investment for a more peaceful, ROI is longer-term.

@akurtakov
Copy link
Member

So where is Java 19 used ?

See above log output: [WARNING] Using JavaSE-19 to fulfill requested profile of JavaSE-17

This is local build on @mickaelistria machine where he has Java 19 as build JVM without toolchains configured. I even run Java 20 locally. See that there is no such thing on jenkins at https://ci.eclipse.org/platform/job/eclipse.platform.ua/job/master/121/consoleFull . IMHO having such a huge difference between developers` jvm and bulid jvm is already a problem that's worth reducing to get local and official build environments closer to each other to reduce surprises.

@laeubi
Copy link
Contributor

laeubi commented Mar 14, 2023

This probably depends on the developer, I have toolchain setup from Java 1.6 to Java 19 at the moment so my IDE/Build can always choose the most suitable one :-)

@laeubi
Copy link
Contributor

laeubi commented Mar 15, 2023

What currently make me wonder specifically is: Why does the API-Tools check want to compile anything at all? Should it not just use the jar build by Tycho?!?

@mickaelistria
Copy link
Contributor Author

Why does the API-Tools check want to compile anything at all? Should it not just use the jar build by Tycho?!?

It cannot use just the jar as it requires the source to see the API annotations inside javadoc.

@laeubi
Copy link
Contributor

laeubi commented Mar 15, 2023

But then how do tools like javadoc work? They seem not to really "compile" the code but can still process source level annotations... I think its fine to process the sources but this seems to compile it again what defeats the purpose of having a compile phase (that already has passed sucessfull).

@mickaelistria
Copy link
Contributor Author

Whether API Tools can work on sources, without requiring the classes would need to be tested. Maybe it can do it already, maybe not. Indeed, if we can skip some useless steps, that would be good.
But API Tools still need classes with resolved bindings to find which methods/types are actually referenced and navigate to their definition. Those bindings do require a properly configured project without JDT errors in it.

@laeubi
Copy link
Contributor

laeubi commented Mar 28, 2023

I have now debugged this a bit further and the classpath contributor is actually called. Strange enough it returns some annotation bundles, but the one for the org.osgi.service.component is missing here even though I can see it in the target platform!

@laeubi
Copy link
Contributor

laeubi commented Mar 28, 2023

Alright the problem is that the org.eclipse.osgi.services also provides the package and therefore is resolved as the provider of the package and therefore then not found by PDE as it searches for the bundle...

@laeubi
Copy link
Contributor

laeubi commented Mar 28, 2023

I now added a workaround here, once this is merged we can finally get rid of these dynamic imports:

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.

4 participants