Bug 363331

Comment 1 Igor Fedorenko 2011-11-10 11:54:16 EST

Implemented, see commit comment for more details.

http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/commit/?id=93b73a0dac384bfa730357a7a223347ec529f12e

Comment 2 Tobias Oberlies 2011-11-11 09:54:07 EST

What is the use case for this change? It seems that the equinox problem could be solved by excluding the bundle org.eclipse.equinox.servletbridge.extensionbundle from the target platform. This is possible with target files with includeMode="slicer".

Obviously, this can be complicated, if you have many dependencies, and you don't care about most of them. But in that case, I would propose to offer the capability to remove units from the target platform.

I don't like the current solution for the following reasons:
- The configuration (and the bug title) implies that you modify the target platform, but this is not the case. Instead, the new parameter affects the p2-based dependency resolution. This is a problem, because not all packaging types need dependency resolution (eclipse-feature when bug 353399 is implemented, and eclipse-repository when we implement the different inclusion policies).
- Through the new parameter, you inject new dependencies into the modules. In case the project already had the dependency (but without version restriction), this is not a problem, but all other modules get a new, artificial dependency. Remember, that we removed the dependency walker but use the entire planner result as project dependencies to be injected into the maven model.

Comment 3 Igor Fedorenko 2011-11-11 13:33:27 EST

Thank you for the review, Tobias, and see my comments inline


(In reply to comment #2)
> What is the use case for this change? It seems that the equinox problem could
> be solved by excluding the bundle
> org.eclipse.equinox.servletbridge.extensionbundle from the target platform.
> This is possible with target files with includeMode="slicer".
> 
> Obviously, this can be complicated, if you have many dependencies, and you
> don't care about most of them. But in that case, I would propose to offer the
> capability to remove units from the target platform.


Sorry, I was little "informal" ;-) in the bug description, so let give more detailed reasoning.

First, I agree that explicit target platform specification (i.e. the .target file) already provides enough control over build target platform contents and only one of .target and <extraRequirements/> should be allowed for a project. I'll update implementation to explicitly forbid use of both sometime next week.

<extraRequirements/> is really meant for when target platform contents is derived from project runtime requirements. 

As I mentioned many times, there is a fundamental dependency between project runtime requirements, which often allow ranges of versions and can even be expressed using more abstract terms like import-package and require-capability, compile-time dependencies must be specific. So the main purpose of <extraRequirements/> is to allow choosing specific compile-time dependencies without having to define .target file and servletbridge.extensionbundle is just one example of that. Dealing with optional dependency is another case when <extraRequirements/> will be useful, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=351842#c9 for more detailed explanation.

> 
> I don't like the current solution for the following reasons:
> - The configuration (and the bug title) implies that you modify the target
> platform, but this is not the case. Instead, the new parameter affects the
> p2-based dependency resolution. This is a problem, because not all packaging
> types need dependency resolution (eclipse-feature when bug 353399 is
> implemented, and eclipse-repository when we implement the different inclusion
> policies).

Ability to modify target platform content *is* the intent of the new parameter. The syntax purposely uses artifact type/id/version terminology as I wanted to emphasize that it is about picking specific dependencies. I kinda agree that "versionRange" looks little out of place, but I wanted to have flexibility to only partially lock dependency and use of "version" for a version range parameter would be misleading.

I need to review the code more carefully, but I believe the same rational is fully applicable to all Tycho projects. Although <extraRequirements> is probably not too useful for eclipse-feature and eclipse-repository project, I do not believe it is harmful either so I do not see a strong reason to limit it to bundles only.

> - Through the new parameter, you inject new dependencies into the modules. In
> case the project already had the dependency (but without version restriction),
> this is not a problem, but all other modules get a new, artificial dependency.
> Remember, that we removed the dependency walker but use the entire planner
> result as project dependencies to be injected into the maven model.

Whether the new parameter applies to all modules or just one is up to the user, so I don't see this to be a problem.

Comment 4 Tobias Oberlies 2011-11-14 09:01:26 EST

I fully agree that your change is addressing a valid use case: We need a way to have some constraints in the dependency resolution without having to fully specify the target platform through the target file. (This is what users expect who know the Maven dependency management capabilities.)

However, I don't think that manipulating the dependency resolution directly is a good idea. Instead, I would offer a possibility to manipulate the target platform. (Note that target platform and dependency resolution are two different things - see a definition here [1].)

The reason for this concern is that we may drop the use of the p2 dependency resolver for eclipse-feature and eclipse-repository, and then the extraRequirements parameter will no longer have an effect (and we will have trouble explaining this).

Therefore I would implement the use case by adding the possibility to remove content from the target platform, e.g. all but one version of some unit. Such a solution would continue to work even with future implementations of eclipse-feature, eclipse-repository, and eclipse-product. Does this become clear, or do you want me to show what I mean in code?

[1] http://wiki.eclipse.org/Tycho/Glossary

Comment 5 Igor Fedorenko 2011-11-14 12:21:20 EST

(In reply to comment #4)
> I fully agree that your change is addressing a valid use case: We need a way to
> have some constraints in the dependency resolution without having to fully
> specify the target platform through the target file. (This is what users expect
> who know the Maven dependency management capabilities.)
> 
> However, I don't think that manipulating the dependency resolution directly is
> a good idea. Instead, I would offer a possibility to manipulate the target
> platform. (Note that target platform and dependency resolution are two
> different things - see a definition here [1].)
> 

I don't understand what you mean. My implementation and indent behind it _is_ to change build target platform contents. It is not supposed to directly change project dependencies. What made you think otherwise?

> The reason for this concern is that we may drop the use of the p2 dependency
> resolver for eclipse-feature and eclipse-repository, and then the
> extraRequirements parameter will no longer have an effect (and we will have
> trouble explaining this).
> 

It's hard to tell anything specific without knowing more about what you want to achieve. At any rate, if we keep "target platform" resolution separate from "dependency" resolution, which I believe we should, then extraRequirements should fit perfectly well into overall scheme of things, regardless if target platform is resolved using p2 or by some other means. 

I actually very consciously picked names of configuration elements that are not directly related to p2, but rather use artifact type/id/version terminology.

> Therefore I would implement the use case by adding the possibility to remove
> content from the target platform, e.g. all but one version of some unit. Such a
> solution would continue to work even with future implementations of
> eclipse-feature, eclipse-repository, and eclipse-product. Does this become
> clear, or do you want me to show what I mean in code?
> 

No, negative requirements is not the right solution to the usecase behind this enhancement. extraRequirements are meant to "help" (or to force) target platform resolver pick specific provider of a capability. Even though the same effect can be achieved by blocking unwanted providers, this, in my opinion, would be both counter-intuitive and error-prone. We still may want allow negative requirements for other reasons, but not for this usecase.

Comment 6 Tobias Oberlies 2011-11-15 05:03:43 EST

We clearly have a naming problem here. Let me try to explain my understanding definition of "target platform" at the example of an eclipse-plugin project:
1. If a target file is specified, the TargetDefinitionResolver resolves the target file
2. ResolutionContextImpl.gatherAvailableInstallableUnits computes the "target platform" (Note: the target platform is independent of the project dependencies - terms like "implicit target platform" or "derived target platform" therefore don't make sense)
3. P2ResolverImpl.resolveProject resolves project dependencies. The result doesn't have a good name - the best I can think of is "resolved project dependencies"
4. The equinox resolver (don't know the class exactly) takes the resolved project dependencies (which is a list of artifacts available on the local hard disk - in contrast to the target platform which is only a list of IUs) and computes the class path from this.

Your change affects step 3, but that step is conceptionally not needed for eclipse-repository and eclipse-product (and possibly eclipse-feature) which should operate directly on the target platform and not on the resolved project dependencies. Currently, all packaging types still use the resolved project dependencies, but this is a conceptional bug that we must get rid of if we want to allow more control over the eclipse-repository content.

Does this make sense to you?

Comment 7 Igor Fedorenko 2011-11-15 12:13:59 EST

(In reply to comment #6)
> We clearly have a naming problem here. Let me try to explain my understanding
> definition of "target platform" at the example of an eclipse-plugin project:
> 1. If a target file is specified, the TargetDefinitionResolver resolves the
> target file

"extraRequirements" and .target files cannot be used together and will result in explicit build failure (see comment 3).

Comment 8 Tobias Oberlies 2011-11-16 02:11:56 EST

(In reply to comment #7)
> "extraRequirements" and .target files cannot be used together and will result in
> explicit build failure (see comment 3).
Why not? It is possible to mix repositories with layout p2 and target files, and the result is well-defined (see [1]).

[1] http://wiki.eclipse.org/index.php?title=Tycho/Target_Platform#Target_platform_configuration

Comment 9 Tobias Oberlies 2011-11-16 02:52:50 EST

We should probably continue the naming discussion to the tycho-dev list.

Btw, the use case discussed here was originally captured in bug 356579.

Comment 10 Tobias Oberlies 2011-11-16 09:06:27 EST

(In reply to comment #5)
> Even though the same effect can
> be achieved by blocking unwanted providers, this, in my opinion, would be both
> counter-intuitive and error-prone. We still may want allow negative requirements
> for other reasons, but not for this usecase.

Note that I am not talking about negative requirements in the p2 sense, but rather about "manually" filtering IUs from the target platform (e.g. in gatherAvailableUnits)

I agree that there are things which cannot be done through filtering the target platform content: for example restricting a bundle A to one version will not not restrict the allowed versions of bundle B even if A has a version range dependency on B. (But AFAIK this is consistent with Maven dependency management.)

But the advantage of filtering the target platform is that it will affect everything in the build that uses the target platform, and not only things that work on the resolved project dependencies. (See [1] for examples of things which should work on the target platform directly.)

[1] http://dev.eclipse.org/mhonarc/lists/tycho-dev/msg00347.html

Comment 11 Igor Fedorenko 2011-11-16 09:43:18 EST

(In reply to comment #10)
> 
> But the advantage of filtering the target platform is that it will affect
> everything in the build that uses the target platform, and not only things that
> work on the resolved project dependencies. (See [1] for examples of things
> which should work on the target platform directly.)
> 

Sure, there are cases when filtering can be useful and convenient but in this case, however, the user really wants to pick specific provider of a required capability and I believe configuration syntax should directly reflect this.

Comment 12 Igor Fedorenko 2011-11-16 09:46:18 EST

(In reply to comment #8)
> (In reply to comment #7)
> > "extraRequirements" and .target files cannot be used together and will result in
> > explicit build failure (see comment 3).
> Why not? It is possible to mix repositories with layout p2 and target files,
> and the result is well-defined (see [1]).
> 
> [1]
> http://wiki.eclipse.org/index.php?title=Tycho/Target_Platform#Target_platform_configuration

I believe changes to dependency resolution logic introduced in 0.12 and 0.13 introduced unnecessary "morphing" between use of .target files and <repository/> elements and we need to cleanup it. I'll follow up on this on tycho-dev.

Comment 13 Tobias Oberlies 2011-11-17 10:19:56 EST

(In reply to comment #11)
> Sure, there are cases when filtering can be useful and convenient but in this
> case, however, the user really wants to pick specific provider of a required
> capability and I believe configuration syntax should directly reflect this.
So your aim is to manage the provider (artifact&version) of one required capability? Then we really need target platform filtering. (Note: with "filtering" I also mean "remove all but one provider".) Your patch doesn't achieve this goal.

Comment 14 Igor Fedorenko 2011-11-17 11:46:54 EST

(In reply to comment #13)
> (In reply to comment #11)
> > Sure, there are cases when filtering can be useful and convenient but in this
> > case, however, the user really wants to pick specific provider of a required
> > capability and I believe configuration syntax should directly reflect this.
> So your aim is to manage the provider (artifact&version) of one required
> capability? Then we really need target platform filtering. (Note: with
> "filtering" I also mean "remove all but one provider".) Your patch doesn't
> achieve this goal.

Complete implementation of provider filtering will require both ability to explicitly exclude unwanted providers and ability to explicitly include desired ones. I've implemented "include" part, which solves the immediate problem I have. I can implement "exclude" part too, but I am reluctant to do this without specific usecase infront of me. "extraRequirements" is probably not the best name in this context and I am happy to change it if somebody can suggest a better one ;-)

Comment 15 Tobias Oberlies 2011-11-18 04:35:27 EST

(In reply to comment #14)
> Complete implementation of provider filtering will require both ability to
> explicitly exclude unwanted providers and ability to explicitly include desired
> ones. I've implemented "include" part, which solves the immediate problem I
> have. I can implement "exclude" part too, but I am reluctant to do this without
> specific usecase infront of me. 
This makes sense, an I can understand your use case. I just have concerns about the way it was implemented.

Would it be okay for if I changed the implementation in a way to address my concerns, and still cover your use case?

Comment 16 Igor Fedorenko 2011-11-18 10:52:37 EST

(In reply to comment #15)
> (In reply to comment #14)
> > Complete implementation of provider filtering will require both ability to
> > explicitly exclude unwanted providers and ability to explicitly include desired
> > ones. I've implemented "include" part, which solves the immediate problem I
> > have. I can implement "exclude" part too, but I am reluctant to do this without
> > specific usecase infront of me. 
> This makes sense, an I can understand your use case. I just have concerns about
> the way it was implemented.
> 
> Would it be okay for if I changed the implementation in a way to address my
> concerns, and still cover your use case?

Lets decide what we want to do about target platform in general first, then either of us can change implementation accordingly.

Comment 17 Tobias Oberlies 2011-12-19 11:44:14 EST

In the phone call on 21. Nov, we agreed to move the extraRequirements parameter to the tycho-compiler-plugin. The reason is that with the new definition of "target platform" (bug 364134), the extra requirements setting doesn't affect the target platform, but only project dependency resolution, e.g. for the compiler. In lack of a better place, the extraRequirements parameter shall be moved to the tycho-compiler-plugin. This is in analogy to the optionalDependencies parameter, which shall also be moved to the tycho-compiler-plugin (see bug 351842 comment 27).

I have a patch for the extraRequirements movement which is almost ready to be committed.

Comment 18 Tobias Oberlies 2011-12-29 13:12:43 EST

(In reply to comment #17)
> In the phone call on 21. Nov, we agreed to move the extraRequirements parameter
> to the tycho-compiler-plugin.
This is done with b0c912b.

Apart from moving the parameter, I also changed its behaviour to only affect dependency resolution & compilation, but not the test runtime. Having side-effects from the compiler configuration to the test runtime seems strange...

@Igor: Did the integration test reflect your use case for the extraRequirements parameter? If yes, could you please try out the new target platform filtering mechanism (bug 356579)? Apart from the pom configuration, the integration test /tycho-its/projects/target.restriction.filter is the same as the test you originally wrote for this bug.

I had to change the integration test for extraRequirements, because it tested the effect on the test runtime (which is no longer there). I don't really like the new test because it uses remote repositories and is quite artificial.

@Igor: Do you have a good use case for the new extraRequirements parameter? I vaguely remember something like jars.extra.classpath with version range. Is this correct? Do you want to change the integration test to be closer to your use case?

Comment 19 Igor Fedorenko 2011-12-29 13:41:57 EST

(In reply to comment #18)
> (In reply to comment #17)
> > In the phone call on 21. Nov, we agreed to move the extraRequirements parameter
> > to the tycho-compiler-plugin.
> This is done with b0c912b.
> 
> Apart from moving the parameter, I also changed its behaviour to only affect
> dependency resolution & compilation, but not the test runtime. Having
> side-effects from the compiler configuration to the test runtime seems
> strange...
> 
> @Igor: Did the integration test reflect your use case for the extraRequirements
> parameter? If yes, could you please try out the new target platform filtering
> mechanism (bug 356579)? Apart from the pom configuration, the integration test
> /tycho-its/projects/target.restriction.filter is the same as the test you
> originally wrote for this bug.
> 
> I had to change the integration test for extraRequirements, because it tested
> the effect on the test runtime (which is no longer there). I don't really like
> the new test because it uses remote repositories and is quite artificial.
> 
> @Igor: Do you have a good use case for the new extraRequirements parameter? I
> vaguely remember something like jars.extra.classpath with version range. Is
> this correct? Do you want to change the integration test to be closer to your
> use case?

Please rollback. You can't just change the integration tests "you do not like".

Comment 20 Tobias Oberlies 2011-12-30 05:01:37 EST

(In reply to comment #19)
> Please rollback. You can't just change the integration tests "you do not like".
I think we have a misunderstanding here: The integration test that restricts the version of org.eclipse.osgi to the version range [3.4,3.5) [1] still exists as [2]. It uses a different POM configuration, but the use case is covered. Is it unclear how to convert the old extraRequirements to filters?

The new target platform filtering mechanism (bug 356579) address the use cases you described in comment 3 and comment 11. I don't have a good use case for extraRequirements which is not better implemented with filters. Therefore, I couldn't think of a good integration test - then new integration test [3] "solves" a problem that no-one would solve this way. This is why I don't like my new integration test, and was wondering if you have an additional use case for extraRequirements which is not version restriction.

About the behaviour change of extraRequirements: I am open to have extraRequirements also affect the dependency resolution for the test runtime - but I would like to see a use case for this first.

[1] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/tree/tycho-its/projects/363331_extraTargetPlatformRequirements?id=93b73a0dac384bfa730357a7a223347ec529f12e
[2] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/tree/tycho-its/projects/target.restriction.filter
[3] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/tree/tycho-its/projects/363331_extraTargetPlatformRequirements

Comment 21 Igor Fedorenko 2011-12-30 07:40:52 EST

(In reply to comment #20)
> (In reply to comment #19)
> > Please rollback. You can't just change the integration tests "you do not like".
> I think we have a misunderstanding here: The integration test that restricts
> the version of org.eclipse.osgi to the version range [3.4,3.5) [1] still exists
> as [2]. It uses a different POM configuration, but the use case is covered. Is
> it unclear how to convert the old extraRequirements to filters?
> 
> The new target platform filtering mechanism (bug 356579) address the use cases
> you described in comment 3 and comment 11. I don't have a good use case for
> extraRequirements which is not better implemented with filters. Therefore, I
> couldn't think of a good integration test - then new integration test [3]
> "solves" a problem that no-one would solve this way. This is why I don't like
> my new integration test, and was wondering if you have an additional use case
> for extraRequirements which is not version restriction.

Filtering removes IUs from the "dumb" target platform. extraRequirements adds new IUs to the "resolved" project dependencies. The two mechanisms are complimentary but one cannot be converted to another.

> About the behaviour change of extraRequirements: I am open to have
> extraRequirements also affect the dependency resolution for the test runtime -
> but I would like to see a use case for this first.

The usecase explained in comment #3 above.

Changing behaviour captured in an integration test without prio discussion is simply unacceptable. Although I agree there is a room for improvement in test dependency configuration and calculation, this cannot happen as a side-effect of a semi-related change and without prio discussion. Please restore the behaviour captured in the test.

Comment 22 Tobias Oberlies 2011-12-30 09:23:08 EST

(In reply to comment #21)
> Filtering removes IUs from the "dumb" target platform. extraRequirements adds
> new IUs to the "resolved" project dependencies. The two mechanisms are
> complimentary but one cannot be converted to another.
Yes and no. Only in corner cases they behave differently:
- If the module doesn't declare a dependency to the configured unit, extraRequirements adds the unit to the resolved project dependencies; filters do nothing
- If the module declares a dependency with conflicting version, the build fails in case of filters and fails sometimes in case of extraRequirements (depending on singletons)

In the common case, i.e. an open dependency is restricted through the POM, they behave exactly the same. The effect of the configuration in the filters integration test [2] is exactly the same as the extraRequirements configuration in your original test [1].

So since I don't seem to get it, could you please provide a standalone example that cannot be implemented with the functionality available in 3f0599b?

Comment 23 Igor Fedorenko 2012-01-02 07:54:45 EST

I plan to work on a more elaborate dependency filtering/constraining implementation for the next few days.

Comment 24 Igor Fedorenko 2012-01-29 17:28:20 EST

Restored extra-requirements to affect test runtime resolution. Introduced a number of tests that demonstrate the scenarios where explicit configuration is required to get desired test runtime contents (mostly limitations of osgi->p2 dependency mapping).

Moved <extraRequirements> and <optionalDependencies> under new <dependency-resolution> subelement of target-platform-configuration. This should make it reasonable obvious that these parameters apply to dependency resolution. The long-term plan is to replace target-platform-configuration with a generic tycho-configuration configuration block.

http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/commit/?id=a62c91c093b5d6d1698b520ff428de9cf18c2045