Bugzilla – Full Text Bug Listing
| Summary: | Optional dependencies are missing on compile class path | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Steffen Pingel <steffen.pingel> |
| Component: | Tycho | Assignee: | Igor Fedorenko <igor> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | anthony.dahanne, berndvogt, bugs.eclipse.org, cdtdoug, christian.georgi, gernot, gregory.amerson, igor, j.inowolski, kai, kdevolder, kim.moir, lars.martin, mail, mistria, pascal.leclercq, pwebster, robert.sauer, sbouchet, t-oberlies, tonny.madsen, wim.jongman |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 363950, 364475 | ||
| Bug Blocks: | 323902 | ||
|
Description
Steffen Pingel
Failing build: https://hudson.eclipse.org/hudson/user/spingel/my-views/view/Mylyn/job/mylyn-builds-nightly/1/console This is definitely a bug: at compilation time, one typically wants to pull in as many optional dependencies as possible (unless this is prevented through an explicit target platform) and make them available to the compiler. This is necessary to enable the (valid) use case of compiling parts of a bundle against optional dependencies. Does using jars.extra.classpath with platform:/plugins/<bundle ID> [1] work in your case? Obviously this is only a workaround, but it is important that it works without explicit target platform, because with the work planned to fix bug 348933, the explicit target platform will behave the same way as explicit target platforms do today: Both will use a p2 resolver to resolve only the project dependencies. [1] http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.pde.doc.user%2Freference%2Fpde_feature_generating_build.htm Thanks for the tip! I was able to work around the problem by adding the following line to the build.properties: jars.extra.classpath = platform:/plugin/org.eclipse.jdt.core,platform:/plugin/org.eclipse.debug.ui,platform:/plugin/org.eclipse.jdt.ui,platform:/plugin/org.eclipse.jdt.junit *** Bug 355947 has been marked as a duplicate of this bug. *** This bug also affects explicit target platform it seems. For example, I had to make this [1] change to m2e to make it build with tycho 0.13. I am marking this for 0.13 as I believe current workaround is good enough (we can always "unmark" ;-) ) This bug only affects dependency resolution, i.e. optional runtime dependencies declared in the manifest don't result in a compile dependency. This is certainly a bug, but I wouldn't consider it as critical. It results in a fairly obvious errors, and there is a reasonable workaround (see comment #2). Target platform resolution is not affected at all for repositories with layout p2 and target files with slicer mode. When using target files with slicer mode, it depends on the way the referenced bundles were built, i.e. if they declare optional greedy or optional non-greedy dependencies. I would consider this a separate enhancement request to offer a new "planner with all greedy" mode for target platform resolution. Changed title from "builds fails when implicit target is used and optional dependency is specified as non greedy in p2.inf" to "Optional dependencies are missing on compile class path". With p2 Juno SR1, all optional runtime dependencies are non-greedy, and hence the dependency resolver planner fails to pick them up. The p2 enhancement proposed in bug 356574 could be a solution for the problem. *** Bug 357543 has been marked as a duplicate of this bug. *** FYI, I made some extensions to the target platform configuration yesterday (see Bug 363331). It is possible to force required additional target platform requirements through pom.xml configuration now. Unlike build.properties, this can be put in pom.xml profiles and I think gets us closer to the ultimate solution to this problem. From what I understand based on this [1] tycho-dev thread, optional dependencies are used for two main reasons. There are "true" optional dependencies, when the code works without the dependency, but provides some additional functionality when the dependency is present. Such "true" optional dependencies are required to compile the code but automated tests should be able to decide if such dependencies should be included or not, so both full and reduced functionality can tested. Another use for optional dependencies is to emulate "one-from-several-alternatives", like is the case when projects need to provide different logic for different eclipse versions. The code uses reflection or some other trickery to compensate for API differences and will compile even when none of the optional dependencies is present. Tests are run against each of the optional dependencies separately. There is actually another variation of "one-from-several-alternatives" scenario, where all optional dependencies must be present at compile time. Since alternatives are very likely to be mutually exclusive, I do not see how to make this work with p2 target platform resolver. The only solution here seems to be to split the code into several fragments. With this in mind I would like to suggest the following solution By default, Tycho will work for "true" optional dependencies, i.e. both compile classpath and test runtime will require direct optional dependencies to be present by default. New target platform platform configuration boolean parameter <includeOptionalDependencies/> will be used to exclude optional dependencies from test runtime. The same <includeOptionalDependencies/> parameter together with <extraRequirements/> will be used to support "one-from-several-alternatives", most likely from multiple pom.xml profiles, i.e. one profile for each alternative. What do you think? [1] http://dev.eclipse.org/mhonarc/lists/tycho-dev/msg00231.html (In reply to comment #9) > By default, Tycho will work for "true" optional dependencies, i.e. both compile > classpath and test runtime will require direct optional dependencies to be > present by default. Actually, I was more thinking about making the bundle's optional dependencies greedy for dependency resolution. In that way, one can get either setup depending on whether the optionally required unit is present in the target platform or not. (In reply to comment #10) > (In reply to comment #9) > > By default, Tycho will work for "true" optional dependencies, i.e. both compile > > classpath and test runtime will require direct optional dependencies to be > > present by default. > Actually, I was more thinking about making the bundle's optional dependencies > greedy for dependency resolution. In that way, one can get either setup > depending on whether the optionally required unit is present in the target > platform or not. Optional/greedy would work fine with explicit target platform specification, but would result in unnecessary flaky behaviour when target platform is calculated based on bundle manifest. Treating optional dependencies as required/greedy results in more predictable behaviour by default, without limiting neither the flexibility of implicit target platform specification nor control over target platform provided by the use of .target file. (In reply to comment #11) > Treating optional dependencies as required/greedy results in more predictable behaviour by default This is true, and I am usually also a fan of fail early - but in this case I would say that it is sufficient to get a compile error if the optional runtime dependency is mandatory at compile time. Once this is working, I wouldn't expect the optional dependency to disappear again even if the target platform is a bunch of p2 repositories - removing content from a p2 repository is not a valid operation for any serious repository. Still, making optional runtime dependencies mandatory for compile time isn't much more effort either. We just need an additional switch if the optional runtime dependencies shall be ignored at compile time. (In reply to comment #12) > > Still, making optional runtime dependencies mandatory for compile time isn't > much more effort either. We just need an additional switch if the optional > runtime dependencies shall be ignored at compile time. I know that in the SDK, many of our optional dependencies are mandatory at compile time and simply don't get loaded at runtime if not available. PW (In reply to comment #13) > I know that in the SDK, many of our optional dependencies are mandatory at > compile time and simply don't get loaded at runtime if not available. But do you also have optional dependencies which are not even physically available at compile time? In this situation, dependency resolution would fail if we were to treat all optional dependencies as mandatory at compile time. No, we don't have optional dependencies which are not even physically available at compile time. (In reply to comment #12) > (In reply to comment #11) > > Treating optional dependencies as required/greedy results in more predictable behaviour by default > This is true, and I am usually also a fan of fail early - but in this case I > would say that it is sufficient to get a compile error if the optional runtime > dependency is mandatory at compile time. Once this is working, I wouldn't > expect the optional dependency to disappear again even if the target platform > is a bunch of p2 repositories - removing content from a p2 repository is not a > valid operation for any serious repository. > I am more concerned about pom.xml <repository/> elements, especially when they are inherited from a corporate parent pom.xml. For larger projects it is not always obvious why certain repositories are present and it maybe tempting to "clean" unused repositories. I would like to see fast and hopefully clear error message in this case. > Still, making optional runtime dependencies mandatory for compile time isn't > much more effort either. We just need an additional switch if the optional > runtime dependencies shall be ignored at compile time. Yup, this is exactly what I am suggesting we do (sorry if this was not clear from comment 9). Unless there are objections, I'll add new target platform configuration parameter "optationDependencies". It will accept two values "require" (the default) and "ignore". This is I believe will cover all usecases we've discussed so far, but we can always add "optional-greedy" value later if we really need to. (In reply to comment #16) > Unless there are objections, I'll add new target platform configuration > parameter "optationDependencies". It will accept two values "require" (the > default) and "ignore". This is I believe will cover all usecases we've discussed > so far, but we can always add "optional-greedy" value later if we really need > to. Shouldn't this be a setting of the tycho-compiler-plugin? The parameter doesn't change the target platform, but only whether optional dependencies are used for compilation or not. I am asking, because the test runtime should have a different default. In the test runtime, I would not include optional dependencies by default because adding content to the test runtime is possible whereas removing is not. (In reply to comment #17) > (In reply to comment #16) > > Unless there are objections, I'll add new target platform configuration > > parameter "optationDependencies". It will accept two values "require" (the > > default) and "ignore". This is I believe will cover all usecases we've discussed > > so far, but we can always add "optional-greedy" value later if we really need > > to. > Shouldn't this be a setting of the tycho-compiler-plugin? The parameter doesn't > change the target platform, but only whether optional dependencies are used for > compilation or not. > tycho-compiler-plugin has to use the same project dependencies as all other maven plugins that operate on the project. This is important for javadoc-plugin, for example. > I am asking, because the test runtime should have a different default. In the > test runtime, I would not include optional dependencies by default because > adding content to the test runtime is possible whereas removing is not. Tests are always in separate projects, so it is possible to manage target platform configuration for them independently. I am guessing to fully support testing of optional dependencies we need to be able to run tests with and without optional dependencies... but I'll leave this enhancement for later. I've pushed proposed implementation along with unit and integration tests to http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/log/?h=351842_optionalDependencies and opened bug 363950 to track required p2 changes (hint: are any of p2 committers are affected by this problem? ;-) ) (In reply to comment #18) > > Shouldn't this be a setting of the tycho-compiler-plugin? > > tycho-compiler-plugin has to use the same project dependencies as all other > maven plugins that operate on the project. This is important for javadoc-plugin, > for example. But the compliler plugin is leading: We want the dependencies in the Maven model to match (as good as possible) what the tycho-compiler-plugin is using. This is what is important for interoperability with other plugins. The fact that Tycho is using a p2 resolver for this is rather an implementation detail that should be transparent, i.e. the user should not need to provide parameters to the p2 resolver directly. (In reply to comment #20) > (In reply to comment #18) > > > Shouldn't this be a setting of the tycho-compiler-plugin? > > > > tycho-compiler-plugin has to use the same project dependencies as all other > > maven plugins that operate on the project. This is important for javadoc-plugin, > > for example. > But the compliler plugin is leading: We want the dependencies in the Maven > model to match (as good as possible) what the tycho-compiler-plugin is using. > This is what is important for interoperability with other plugins. The fact > that Tycho is using a p2 resolver for this is rather an implementation detail > that should be transparent, i.e. the user should not need to provide parameters > to the p2 resolver directly. I was not suggesting to require the user configure p2 resolver directly. I was saying that compiler plugin is just one consumer of the resolved project dependencies, there are other plugins that need to access dependencies and therefore resolution configuration can't be specific to the compiler plugin. Found another case when Tycho was not properly handling optional dependencies. Opened bug 364475 to track required p2 changes. (In reply to comment #22) > Found another case when Tycho was not properly handling optional dependencies. > Opened bug 364475 to track required p2 changes. Rather than hooking into the publisher actions, we could also just take the original actions and edit the resulting IUs for dependency resolution, right? We are anyway only talking about the "dependency only" units which don't end up in the build result. IMHO this is easier to implement. (In reply to comment #23) > (In reply to comment #22) > > Found another case when Tycho was not properly handling optional dependencies. > > Opened bug 364475 to track required p2 changes. > Rather than hooking into the publisher actions, we could also just take the > original actions and edit the resulting IUs for dependency resolution, right? > We are anyway only talking about the "dependency only" units which don't end up > in the build result. IMHO this is easier to implement. Neither IInstallableUnit nor IRequirement expose required mutators, so we'd have to cast to specific implementations to change generated metadata. I don't feel particularly strong about this, but using semi-official APIs seems like a better approach. In any case, feel free to change the code, as long as the unit tests pass I don't mind ;-) Is this "optionalDependencies=require" fix already available in a SNAPSHOT build? Nebula uses lots of optionalDependencies... (In reply to comment #25) > Is this "optionalDependencies=require" fix already available in a SNAPSHOT > build? Nebula uses lots of optionalDependencies... No, not yet. You'll have to build 351842_optionalDependencies branch yourself (and p2 with required patches before that). This should be fixed now. By default Tycho will require all optional dependencies and will fail the build with explicit error message if any of optional dependencies is missing. The default can be changed with new <optionalDependencies> target-platform-configuration parameter. If set to <optionalDependencies>ignore</optionalDependencies>, Tycho will completely ignore optional dependencies. optionalDependencies=ignore together with <extraRequirements/> introduced by bug 363331 and maven profiles can be used to support "one-from-several-alternatives" scenario. Couple of caveats. First, due to delays in getting required p2 patches accepted (see bug 364221, bug 364222 and bug 364475), Tycho is currently using special p2 build. We plan to switch to official P2 builds as soon as there is one we can use. Second, we are still discussing the best location for <optionalDependencies> and <extraRequirements/> configuration parameters, so you may have to update your pom.xml files. For the brave souls that are willing to try this fix, just set tycho version to 0.14.0-SNAPSHOT and add the following to your (parent) pom.xml file <pluginRepositories> <pluginRepository> <id>tycho-snapshots</id> <url>https://repository.sonatype.org/content/repositories/snapshots/</url> </pluginRepository> </pluginRepositories> As usual, please provide feedback, good or otherwise, on tycho-dev or tycho-user mailing lists. http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/commit/?id=c4aafe3bbfd589f992697b3a0c6dd33a4c553187 Tried this for the Nebula use-case, and default (no-options) behavior works like a charm. I tried to build a ~600 plugins project and while I'm still running into problems, all issues with regard to optional dependencies look resolved to me. Before the latest fix, I tried with little success to get things running with a mixture of jar.extra.classpath and the fix of https://bugs.eclipse.org/bugs/show_bug.cgi?id=363331, with no success. Only with the latest changes, everything works out of the box. (In reply to comment #27) > Second, we are still discussing the best location for <optionalDependencies> and > <extraRequirements/> configuration parameters, so you may have to update your > pom.xml files. The optionalDependencies parameter was moved to the tycho-compiler-plugin (as well as the extraRequirements parameter; cf. bug 363331). So if you need to set optionalDependencies=ignore, please follow the example of this integration test [1]. You do not need any configuration for the default value optionalDependencies=require. The reason behind this move is that the parameter only affects the resolution of the class path, and for example not the test runtime. Also, I am trying to establish a simpler definition of "target platform" (which hopefully makes understanding Tycho easier; cf bug 364134). With the new definition [2] the optionalDependencies parameter does not affect the target platform, so the parameter shouldn't be in target-platform-configuration. [1] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/tree/tycho-its/projects/compiler.optionalDependencies/require-bundle-ignore/pom.xml [2] http://wiki.eclipse.org/Tycho/Target_Platform (In reply to comment #30) > (In reply to comment #27) > > Second, we are still discussing the best location for <optionalDependencies> and > > <extraRequirements/> configuration parameters, so you may have to update your > > pom.xml files. > The optionalDependencies parameter was moved to the tycho-compiler-plugin (as > well as the extraRequirements parameter; cf. bug 363331). So if you need to set > optionalDependencies=ignore, please follow the example of this integration test > [1]. You do not need any configuration for the default value > optionalDependencies=require. > > The reason behind this move is that the parameter only affects the resolution > of the class path, and for example not the test runtime. Also, I am trying to > establish a simpler definition of "target platform" (which hopefully makes > understanding Tycho easier; cf bug 364134). With the new definition [2] the > optionalDependencies parameter does not affect the target platform, so the > parameter shouldn't be in target-platform-configuration. > > [1] > http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/tree/tycho-its/projects/compiler.optionalDependencies/require-bundle-ignore/pom.xml > [2] http://wiki.eclipse.org/Tycho/Target_Platform -1. please rollback. optionalDependencies must apply to test target runtime. the usecase explained in comment #9. (In reply to comment #31) > optionalDependencies must apply to test target runtime. the usecase explained in > comment #9. Sorry, I was wrong. The test runtime uses the dependency-only metadata, right? So optionalDependencies does affect the test runtime. IMHO this eventually needs to be configurable separately, but this clearly is not within the scope of this bug. In any case, I didn't change any behaviour of the parameter. (In reply to comment #32) > (In reply to comment #31) > > optionalDependencies must apply to test target runtime. the usecase explained in > > comment #9. > Sorry, I was wrong. The test runtime uses the dependency-only metadata, right? > So optionalDependencies does affect the test runtime. IMHO this eventually > needs to be configurable separately, but this clearly is not within the scope > of this bug. > > In any case, I didn't change any behaviour of the parameter. Thank you for the clarification. optionalDependencies must apply to the calculation of the test target runtime. This is required to support "one-from-several-alternatives" scenario. Just to be safe, I'll setup an IT to explicitly validate this. I do agree that it may be desirable to configure optionalDependencies for tests independently and I agree that this additional configuration flexibility is outside of the scope of this bugreport. I can confirm that optionalDependencies does control contents of the test runtime [1]. I do find it counter-intuitive that tycho-compiler-plugin configuration affects test runtime and I have sent an email to tycho-dev and tycho-user mailing list to see what other think. (eclipse mailing lists are down at the moment, it seems). [1] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/commit/?id=fb6634ca69b3308b6cbda0198103e629e4b24346 (In reply to comment #34) > I can confirm that optionalDependencies does control contents of the test > runtime [1]. But it doesn't do this reliably: The optional dependencies of other modules are only affected if they are built in the same reactor. This may lead to a different test behaviour if a test module is built standalone. I've opened bug 367701 to track this problem. Using tycho 0.15, it seems this type of problem still exists if a optional dependency is marked as 'non-greedy' in p2.inf. I.e. we have some dependencies like these: requires.0.namespace=osgi.bundle requires.0.name=org.eclipse.m2e.core requires.0.greedy=false requires.0.optional=true And when we compile bundles that have dependencies like these, we get compile errors because m2e.core stuff is not on the classpath. The workaround of using extraClasspath in build.properties to force these bundles onto the compiler classpath seems to work. However setting optionalDependencies to require has no effect (well, I guess we wouldn't expect it to, since 'require' is already the default, but I tried to set it explicitly anyway). (In reply to comment #36) > Using tycho 0.15, it seems this type of problem still exists if a optional > dependency is marked as 'non-greedy' in p2.inf. > > I.e. we have some dependencies like these: > requires.0.namespace=osgi.bundle > requires.0.name=org.eclipse.m2e.core > requires.0.greedy=false > requires.0.optional=true > > And when we compile bundles that have dependencies like these, we get > compile errors because m2e.core stuff is not on the classpath. > > The workaround of using extraClasspath in build.properties to force these > bundles onto the compiler classpath seems to work. > > However setting optionalDependencies to require has no effect (well, I guess > we wouldn't expect it to, since 'require' is already the default, but I > tried to set it explicitly anyway). This is tracked as bug 379425 Thanks for the pointer. I see that bug also has another workaround to try. That is good because... I discovered there's a caveat with the 'extraClasspath' workaround. It seems like adding the 'extraClasspath' via build.properties bypasses version constraints specified in manifest.mf. (i.e I guess the dependency in manifest.mf is not considered because it is non-greedy, and the build.properties only specifies a plugin name, not a version). So the workaround didn't quite work for us as we ended up with the wrong version of m2e.core on the compile-time classpath, still resulting in compile errors. |