eclipse-equinox / equinox.bundles

Eclipse Public License 2.0
8 stars 16 forks source link

Add missing Package-Import of org.osgi.util.function #31

Closed HannesWell closed 2 years ago

HannesWell commented 2 years ago

Attempt to fix the compilation error in the current I-Build: https://ci.eclipse.org/releng/job/I-build-4.24/87

Without this change I have a very similar compilation error in org.osgi.util.pushstream.AbstractPushStreamImpl like in the one causing the build failure. That class is in the same bundle like the failing org/eclipse/equinox/internal/log/stream/LogStreamProviderFactory.

@sravanlakkimsetti this could help in case the I-build fails again.

HannesWell commented 2 years ago

I'm very sure the failure is related to https://github.com/eclipse-equinox/equinox.framework/pull/41, but I wonder how the plug-in was compiled successfully before without the Package-Import added with this change. To me it looks like the Package-Import of org.osgi.util.function was implicitly exposed to org.eclipse.equinox.log.stream since it was wired to org.eclipse.osgi.util due some other package-import. But from understanding this must not happen because it would contradict the purpose of Import-Package.

The failure in my workspace is a bit different but looks similar. Unlike the build it shows compile-errors in org.osgi.util.pushstream.AbstractPushStreamImpl which has imports like:

import org.osgi.util.function.Function;
import org.osgi.util.function.Predicate;

@tjwatson can you explain why this plug-in compiled before?

I first assumed that this is due to some multi-repo build magic and thought it would vanish automatically once the linked change was build. Looks like I was wrong. Sorry for the inconvenience.

laeubi commented 2 years ago

I wonder how the plug-in was compiled successfully before without the Package-Import added with this change. To me it looks like the Package-Import of org.osgi.util.function was implicitly exposed to org.eclipse.equinox.log.stream since it was wired to org.eclipse.osgi.util due some other package-import. But from understanding this must not happen because it would contradict the purpose of Import-Package.

That's a topic we also regularly have at Tycho, there is a difference between compilation and runtime the compiler don't care about the classspace much (first simply win) but needs to know all (transitive) types and the runtime don't care about compile time dependencies (e.g. component annotations or transitive types). But as Tycho resolves the compile-path based on the imports and adds access-rules you will see such strange errors here and there, especially if you try not using require-bundle.

Sadly this leads so some unnecessary imports (and PDE even detect they are not strictly required), but it seems people are okay with that, or even don't care enough to fix EJC/JDT/PDE related issues in that area.

tjwatson commented 2 years ago

@tjwatson can you explain why this plug-in compiled before?

I don't have clear explanation, but I will state that after updating my target and Eclipse to the latest I-Build my Equinox workspace with the org.eclipse.equinox.log.stream project would not compile until I pulled in this fix.

I recall having a similar discussion around compilation of bundles that import javax.servlet.http and the fact that they needed to also import javax.servlet, otherwise ECJ would fail to compile Servlet implementations. I was trying to search for the bug where this long discussion happened, but have not found it yet.

HannesWell commented 2 years ago

@tjwatson can you explain why this plug-in compiled before?

I don't have clear explanation, but I will state that after updating my target and Eclipse to the latest I-Build my Equinox workspace with the org.eclipse.equinox.log.stream project would not compile until I pulled in this fix.

Yes I noticed that as well. But assumed that this will vanish after the build is completed because I also had problems with my Oomph setup that failed to resolve the TP after I have updated some Plug-in/Feature versions because it was looking for specific build-qualifiers. Therefore I ignored that error. But I will pay better attention next time such error occurs. Sorry for the inconvenience that caused for others.

I recall having a similar discussion around compilation of bundles that import javax.servlet.http and the fact that they needed to also import javax.servlet, otherwise ECJ would fail to compile Servlet implementations. I was trying to search for the bug where this long discussion happened, but have not found it yet.

If you find it later, I would be interested to read it.

Sadly this leads so some unnecessary imports (and PDE even detect they are not strictly required), but it seems people are okay with that, or even don't care enough to fix EJC/JDT/PDE related issues in that area.

Thanks @laeubi for the explanation. I think this is something we should definitely address, especially if we tell people to use Import-Package because it is the right thing, but then it should also work well.

laeubi commented 2 years ago

Thanks @laeubi for the explanation. I think this is something we should definitely address, especially if we tell people to use Import-Package because it is the right thing, but then it should also work well.

This is nothing specific to import-package, simple reproducer:

  1. Create new bundle
  2. Require-Bundle: org.eclipse.e4.core.contexts;bundle-version="1.9.100"
  3. Create class that uses context injection factory [1]
  4. Will result in error [2]
  5. Add org.eclipse.e4.core.di;bundle-version="1.8.100"
  6. Manifest > Dependencies > Find unused dependencies

@bjhargrave @tjwatson I think the import here is really not necessary as long as i don't directly reference the type org.eclipse.e4.core.di.InjectionException directly (e.g. in a catch clause) or do I miss something?

One gets the same problem if I remember correctly if I use a class that extends something from a non imported package or contains a method that returns a type from a non imported package.

[1] Example Class:

public class ContextFactoryTest {
    public static void main(String[] args) {
        ContextInjectionFactory.inject(null, null);
    }
}

[2] Errors

The project was not built since its build path is incomplete. Cannot find the class file for org.eclipse.e4.core.di.InjectionException. Fix the build path then try building this project test.import Unknown Java Problem

The type org.eclipse.e4.core.di.InjectionException cannot be resolved. It is indirectly referenced from required .class files ContextFactoryTest.java /test.import/src/testme line 7 Java Problem

laeubi commented 2 years ago

By the way if I look at the class path that is used to run such a project it actually contains the DI of course:

javax.inject_1.0.0.v20091030.jar org.eclipse.e4.core.di.annotations/bin javax.annotation_1.3.5.v20200909-1856.jar org.eclipse.osgi/osgi/osgi.annotation.jar org.eclipse.osgi/osgi/j9stubs.jar org.eclipse.osgi/bin org.eclipse.equinox.region/bin org.eclipse.equinox.transforms.hook/bin org.eclipse.equinox.weaving.hook/bin org.eclipse.osgi.compatibility.state/bin org.eclipse.e4.core.di/bin jakarta.servlet-api_4.0.0.jar rg.eclipse.osgi.services/lib/osgi.annotation.jar rg.eclipse.osgi.services/lib/function.interface.jar rg.eclipse.osgi.services/bin org.eclipse.e4.core.contexts/bin opt/eclipse/platform-sdk/ws/test.import/bin testme.ContextFactoryTest

I just don't have any idea how to find out the compile-classpath passed to the ECJ in eclipse ...