Adding the capability to base the "exported" attribute of Eclipse classpath entries on what configuration(s) they came from

[dgileadi]

As described at http://issues.gradle.org/browse/GRADLE-1613 the Eclipse plugin currently always sets the "exported" attribute of generated classpath entries to "true". This patch makes the "exported" attribute depend on which configuration(s) the entry came from, and also adds a nonExportedConfigurations property to EclipseClasspath for declaring configurations whose dependencies shouldn't be exported. By default the non-exported configurations are testCompile and testRuntime.

[mockitoguy]

Hey David. Thanks again for your hard work on fixing this problem. I cannot merge this fix though :(

Basically we no longer want go down the path of adding more & more 'configuration' objects onto the tasks. It is wrong abstraction really, because it is hard to create configurations easily by hand as they are sort of big objects, coupled heavily with gradle core. Most existing places where we configure the task by passing the configuration objects will be rewritten (we'll introduce a DependencySet object or something along those lines). I realize that at the moment configurations are used all over the place to configure tasks (plusConfigurations, minusConfigurations, new rootConfigurations and libConfigurations, etc.) but we need to stop it now because we'll have more problems in refactoring the existing code once we implement the DependencySet.

However, I would really like to see this problem fixed so I have a suggestion :) How about we make the AbstractClasspathEntry (or some other type in this hierarchy) carry the information about the source configuration name? This way the gradle users should be able to change the exported property based on the configuration using the existing whenConfigured hooks.

In general we should fix this problem in a more general fashion so that hooks are not needed. Yet, we should shy away from passing more configurations.

What do you think?

[dgileadi]

It makes sense that we're trying to avoid using configurations in more places. I'm interested to see what the DependencySet approach will look like, and how it will interact with the current dependency syntax.

However I'm afraid I don't agree with the approach you suggest--provide the hooks for Gradle users to fix the problem instead of fixing it ourselves. Additionally, if we put the source configuration's name into AbstractClasspathEntry we're still exposing configuration stuff, not DependencySet stuff. It would just be exposing it in a different place, one that doesn't automatically fix the problem for users. In my opinion it's better to fix the problem now by using what we have, even if it's not optimal, and then once we migrate to the new stuff to migrate the fix along with it.

With my pull request test dependencies won't be exported in Eclipse any more, which correctly matches Gradle/Maven's dependency behavior. I have the additional need with the m2metadata plugin to add a "provided" configuration as something that isn't exported, since that's something Maven has that Gradle doesn't.

Originally my change didn't expose a "nonExportedConfigurations" property; instead I manually checked whether the configuration was a test configuration or had "provided" in its name. However I felt that making it explicitly configurable was more correct. If you like I can put this behavior back, removing the "nonExportedConfigurations" property. However I feel that's a bit hacky. What I don't want to do is put the burden on users for fixing this behavior.

On Jun 17, 2011, at 7:30 AM, szczepiq wrote:

Hey David. Thanks again for your hard work on fixing this problem. I cannot merge this fix though :(

Basically we no longer want go down the path of adding more & more 'configuration' objects onto the tasks. It is wrong abstraction really, because it is hard to create configurations easily by hand as they are sort of big objects, coupled heavily with gradle core. Most existing places where we configure the task by passing the configuration objects will be rewritten (we'll introduce a DependencySet object or something along those lines). I realize that at the moment configurations are used all over the place to configure tasks (plusConfigurations, minusConfigurations, new rootConfigurations and libConfigurations, etc.) but we need to stop it now because we'll have more problems in refactoring the existing code once we implement the DependencySet.

However, I would really like to see this problem fixed so I have a suggestion :) How about we make the AbstractClasspathEntry (or some other type in this hierarchy) carry the information about the source configuration name? This way the gradle users should be able to change the exported property based on the configuration using the existing whenConfigured hooks.

In general we should fix this problem in a more general fashion so that hooks are not needed. Yet, we should shy away from passing more configurations.

What do you think?

Reply to this email directly or view it on GitHub:
#38 (comment)

[mockitoguy]

However I'm afraid I don't agree with the approach you suggest--provide the hooks for Gradle users to fix the problem instead of fixing it ourselves.

I agree. If we can do it without passing more Configurations and
without complicating the ClasspathFactory :)

Additionally, if we put the source configuration's name into AbstractClasspathEntry we're still exposing >configuration stuff, not DependencySet stuff.

True. Just so you know one of our 'general' strategies - for
configuring tasks we want avoid passing 'big' gradle types like
Project, Configuration, etc. It makes it hard to configure the task if
you don't have the instances. Therefore passing configuration name is
fine (just like passing project names is fine).

Originally my change didn't expose a "nonExportedConfigurations" property; instead I manually checked whether the configuration was a test configuration or had "provided" in its name.

I see :) This is not something we want to pull.

Cheers!

However I felt that making it explicitly configurable was more
correct.  If you like I can put this behavior back, removing the
"nonExportedConfigurations" property.  However I feel that's a bit
hacky.  What I don't want to do is put the burden on users for fixing
this behavior.

On Jun 17, 2011, at 7:30 AM, szczepiq wrote:

Hey David. Thanks again for your hard work on fixing this problem. I cannot merge this fix though :(

Basically we no longer want go down the path of adding more & more 'configuration' objects onto the tasks. It is wrong abstraction really, because it is hard to create configurations easily by hand as they are sort of big objects, coupled heavily with gradle core. Most existing places where we configure the task by passing the configuration objects will be rewritten (we'll introduce a DependencySet object or something along those lines). I realize that at the moment configurations are used all over the place to configure tasks (plusConfigurations, minusConfigurations, new rootConfigurations and libConfigurations, etc.) but we need to stop it now because we'll have more problems in refactoring the existing code once we implement the DependencySet.

However, I would really like to see this problem fixed so I have a suggestion :) How about we make the AbstractClasspathEntry (or some other type in this hierarchy) carry the information about the source configuration name? This way the gradle users should be able to change the exported property based on the configuration using the existing whenConfigured hooks.

In general we should fix this problem in a more general fashion so that hooks are not needed. Yet, we should shy away from passing more configurations.

What do you think?

Reply to this email directly or view it on GitHub:
#38 (comment)

Reply to this email directly or view it on GitHub:
#38 (comment)

Szczepan Faber
Principal engineer@gradleware
Lead@mockito

[dgileadi]

Since the configurations passed to nonExportedConfigurations need to be configurations that are already included in the project, would it be acceptable if I changed nonExportedConfigurations to a list of names instead of a list of Configuration objects? That's a simple change, and it seems like it might line up with the general strategy you mentioned. What do you think?

On Jun 17, 2011, at 11:07 AM, szczepiq wrote:

However I'm afraid I don't agree with the approach you suggest--provide the hooks for Gradle users to fix the problem instead of fixing it ourselves.

I agree. If we can do it without passing more Configurations and
without complicating the ClasspathFactory :)

Additionally, if we put the source configuration's name into AbstractClasspathEntry we're still exposing >configuration stuff, not DependencySet stuff.

True. Just so you know one of our 'general' strategies - for
configuring tasks we want avoid passing 'big' gradle types like
Project, Configuration, etc. It makes it hard to configure the task if
you don't have the instances. Therefore passing configuration name is
fine (just like passing project names is fine).

Originally my change didn't expose a "nonExportedConfigurations" property; instead I manually checked whether the configuration was a test configuration or had "provided" in its name.

I see :) This is not something we want to pull.

Cheers!

However I felt that making it explicitly configurable was more
correct. If you like I can put this behavior back, removing the
"nonExportedConfigurations" property. However I feel that's a bit
hacky. What I don't want to do is put the burden on users for fixing
this behavior.

On Jun 17, 2011, at 7:30 AM, szczepiq wrote:

Hey David. Thanks again for your hard work on fixing this problem. I cannot merge this fix though :(

Basically we no longer want go down the path of adding more & more 'configuration' objects onto the tasks. It is wrong abstraction really, because it is hard to create configurations easily by hand as they are sort of big objects, coupled heavily with gradle core. Most existing places where we configure the task by passing the configuration objects will be rewritten (we'll introduce a DependencySet object or something along those lines). I realize that at the moment configurations are used all over the place to configure tasks (plusConfigurations, minusConfigurations, new rootConfigurations and libConfigurations, etc.) but we need to stop it now because we'll have more problems in refactoring the existing code once we implement the DependencySet.

However, I would really like to see this problem fixed so I have a suggestion :) How about we make the AbstractClasspathEntry (or some other type in this hierarchy) carry the information about the source configuration name? This way the gradle users should be able to change the exported property based on the configuration using the existing whenConfigured hooks.

In general we should fix this problem in a more general fashion so that hooks are not needed. Yet, we should shy away from passing more configurations.

What do you think?

Reply to this email directly or view it on GitHub:
#38 (comment)

Reply to this email directly or view it on GitHub:
#38 (comment)

Szczepan Faber
Principal engineer@gradleware
Lead@mockito

Reply to this email directly or view it on GitHub:
#38 (comment)

[mockitoguy]

Sounds good. You can tune the name of the property so that it's clear we need names.

Can you also try implementing it so that there's least impact on the ClasspathFactory? This puppy needs to be refactored soon and it is a bit duplicated with idea & wtp classpath factories. The less changes go there, the easier will be to tidy it up later.

[dgileadi]

I'll go ahead with that.

As far as the impact on ClasspathFactory, I'll see what I can do but as I recall I needed some way of getting the configuration down to the location where entries were created from dependencies, and doing so needed a fair few changes.

Thanks for helping this move forward!

On Jun 17, 2011, at 1:42 PM, szczepiq wrote:

Sounds good. You can tune the name of the property so that it's clear we need names.

Can you also try implementing it so that there's least impact on the ClasspathFactory? This puppy needs to be refactored soon and it is a bit duplicated with idea & wtp classpath factories. The less changes go there, the easier will be to tidy it up later.

Reply to this email directly or view it on GitHub:
#38 (comment)

[dgileadi]

Okay, here's an updated version:

• Renamed the property to noExportConfigNames
  * Reduced the changes to ClasspathFactory

[matthewmccullough]

I need this merged in with @dgileadi's latest adjustments to successfully use his work in https://github.com/jbaruch/Gradle-M2Metadata-Plugin/tree/dgiladiimprovements

[mockitoguy]

Ok, thanks for letting me know. I'll try to merge it.

[mockitoguy]

Fixed :) I inspired on this pull request. Couple of differences from your latest adjustments:

1. I couldn't configure 'provided' configuration always 'noExport' because there's no notion of 'provided' in Gradle at the moment. The only thing that could possible be done I guess is to make the eclipse plugin configure the noExportConfigurations with provided configuration when war plugin is applied. I haven't done it, though - it's just an idea.
2. I reverted to your initial idea of passing the configurations instead of names because it just felt more consistent (you're right :)... Sorry @matthew - it's going to be yet another change in your M2Metadata ;-/
3. I refactored the ClasspathFactory completely to finally reuse logic with idea plugin. After the refactoring it was fairly easy to implement the feature in question: 6627b8b (no changes to the eclipse ClasspathFactory.