stevesaliman / gradle-properties-plugin

Gradle plugin to simplify loading project properties from external environment specific files
Apache License 2.0
192 stars 28 forks source link

Make the plugin applicable to Settings #13

Closed Vampire closed 10 years ago

Vampire commented 10 years ago

This change makes the plugin applicable to a Settings instance in settings.gradle. As far as I can tell, the issue from http://forums.gradle.org/gradle/topics/applying_a_plugin_by_alias_to_settings_gradle is a Gradle bug and not a problem of my implementation, so it would be great if you could merge this change in also before releasing, as it is already usable, you just have to use the FQCN to apply the plugin in settings.gradle as I noted in the README.md.

stevesaliman commented 10 years ago

I have to ask: What is the case for making it possible to apply the plugin from settings.gradle? I don't see a lot of demand for it, and it looks like most plugins don't support this feature, including the Java, Groovy, Scala or Eclipse plugins that come with Gradle itself, or the well known and used grails plugin.

That said, if we want to include this feature, I think there is still some work to be done since applying the plugin from settings.gradle only includes the top and bottom directories in a multi-project build, skipping the files in the middle of a 3 or more tier project.

When building the file list for a project, the plugin starts with the project applying the plugin, and walks up the tree until it gets to the root project. I think we need a similar mechanism for settings.

I think what what we need is to start with the settints.settingsDir, then evaluate each parent directory until we find and process settings.rootDir

We would also need to duplicate all of the tests in the test classes to make sure that they produce the same results when the plugin is applied from a parent or child settings file as they do when the plugin is applied from a project.

Vampire commented 10 years ago

I guess you confuse what the settings.gradle file is. There can only be one settings.gradle file per multi-project build. And it makes absolutely not sense to apply something like the java, groovy, scala, eclipse or grailse plugin.

The settings.gradle file defines which projects are part of a multi-project build and in which parent/child relationship.

Why I added the applicability on settings.gradle is, because I need it. That is in most cases the reason for me to contribute to a project. :-) In my case, we have an XML file that defines per customer which modules have to be part of his build. This file is read in settings.gradle and the according subprojects added. So if a customer only gets 5 out of the many more modules, the build needs only to care about those 5 sub-projects and not check and skip all the other projects that are not part of the build anyway.

For which customer and which environment of a customer a developer or the buildserver builds is controlled via gradle properties. These properties are what I need in settings.gradle and which should be overridable in gradle-local.properties.

You are right that it makes senes for the least Gradle plugins to be applicable to Settings and that it makes sense for even lesser plugins to be applicable to Settings AND Project, but the Properties plugin is one of the examples where it makes sense and I hope you see this now also. :-)

As to which files to include, as I said, you can only have one settings.gradle file per multi-project build, no hierarchy and this file usually lies beneath the build.gradle file of the root project. Afair this is not mandatory and you can request the settings directory and the root project directory separately from the Settings instance. Because of that, I thought it would be appropriate to include the root project property files and that settings directory property files in the logic and that's it.

About unit tests, it would certainly be good to have some tests, but unfortunately I didn't see a way to mock the Settings instance like with ProjectBuilder and I also see no way to retrieve it from the root project or the gradle instance. So I thought it would probably be ok without tests, as the property overwriting mechanism itself is tested against the project and it uses the same codepath. The only difference is that files that are being taken into account.

Vampire commented 10 years ago

While writing about it, I remembered that I forgot to check for equalnes of root project dir and settings dir, so I amended my commit to add that check. It makes not much sense to evaluate the same file twice.

stevesaliman commented 10 years ago

Your reason for getting involved with the project is the same reason I started it in the first place :-)

I'm not particularly opposed to applying this plugin in the settings.gradle file, I just need to make sure I understand how that mechanism is supposed to work so that I can continue to support the code going forward...

I played around a bit with this code last night, creating a 3-level hierarchy of projects, with each one having a settings.gradle file. What I didn't try was doing a build from the grandchild project with no settings.gradle to see if it pulls in the one from the root project.

My main concern with applying from a setting file is that it should process the same files in the same order as it would if it was applied from build.gradle. This is why I thought it should start in the project directory and work up. Since the project doesn't exist during the initialization phase, I need a way to figure out which directory the plugin was applied from. I'll look at it some more this evening.

Have you found any good references for doing things in settings.gradle?

Vampire commented 10 years ago

Your reason for getting involved with the project is the same reason I started it in the first place :-)

That's most often how great projects start. :-)

I'm not particularly opposed to applying this plugin in the settings.gradle file, I just need to make sure I understand how that mechanism is supposed to work so that I can continue to support the code going forward...

Definitely, no veto here. And yes, you should get a deeper understanding of it. Unfortunately I think you didn't get yet how settings.gradle works.

I played around a bit with this code last night, creating a 3-level hierarchy of projects, with each one having a settings.gradle file. What I didn't try was doing a build from the grandchild project with no settings.gradle to see if it pulls in the one from the root project.

Here is where I think that you didn't yet understand the multi-project structure and settings.gradle file mechanism of Gradle which I tried to explain to you. Having a directory hierarchy A->B->C with each project having a build.gradle file, means you have three independent gradle builds A, B and C and not a multi-project Gradle build. Because of that it is good that you didn't use File.getParentFile() in your plugin, but Project.getParent().getProjectDir(). You can also have three directories beneath each other but with a relationship of parent, child, grand-child. And if you have A->B->C and in each a settings.gradle file, then you also have three independent Gradle builds and not a multi-project Gradle build. If you have A->B->C and a settings.gradle file in A and B but none in C, then it depends on the settings.gradle file in B. If it adds C, then you have a multi-project build with B and C and an independent Gradle build A. If settings.gradle in B does NOT add C, you again have three independent Gradle builds. More typical is that you either have A->B->C with a settings.gradle file in A that adds B and C as child and grandchild, or master, A, B and C on one level with a settings.gradle file in master that adds A, B and C in the wanted hierarchy.

So to summarize it again, you can have at most one settings.gradle file per build. In this settings.gradle file you define which projects are part of the build in which hierarchical order and with which directory layout on the filesystem. In the settings.gradle file you can also change the project directory of the root project and thus have different directory for root project and settings directory, that's why my code pulls in root project property files and settings directory property files. Maybe in which order these should be taken into account can be discussed though.

You can read more about mult-project builds, the settings.gradle file, how it is found if invoking gradle in one of the sub-projects and how it is evaluated in "Chapter 55. The Build Lifecycle" and "Chapter 56. Multi-project Builds" of the Gradle User Guide.

Btw. please be aware that I amended the commit to add an additional if, not that you merge in the old state without the if.

My main concern with applying from a setting file is that it should process the same files in the same order as it would if it was applied from build.gradle. This is why I thought it should start in the project directory and work up. Since the project doesn't exist during the initialization phase, I > need a way to figure out which directory the plugin was applied from. I'll look at it some more this evening.

I hope my explanation and probably the two User Guide chapters made it clear that this is not correct. :-)

Have you found any good references for doing things in settings.gradle?

Besides the two User Guide chapters I mentioned above?

stevesaliman commented 10 years ago

Some light reading for the weekend :-)

I've seen these chapters before, and used them to work out the multi-project behavior, but It looks like they've flushed this out more since I read it last. In particular, I need to understand the difference between multiple related projects being built in a multi-project build and multiple independent projects being built...

Vampire commented 10 years ago

Basically what I explained above. And for this discussion the only interesing fact is, that you must have exactly ONE settings.gradle file in your multi-project build and you CAN have at most one settings.gradle file in your single-project build. This settings.gradle file lies either in the root folder of your project hierarchy or in a directory master if you maintain a flat project directory layout and that by default the location where the settings.gradle file lies is also the project directory of the root project but that could be redefined by the settings.gradle file.

stevesaliman commented 10 years ago

I think the part I was missing was the fact that settings and project instances are completely separate. Applying the plugin in settings.gradle has nothing to do with projects or their properties, and only set properties to be used in settings.gradle itself. It is almost as if we have 2 plugins: a project properties plugin and a settings properties plugin, they just happen to run with the same code.

I've been experimenting with different project configurations, and I have learned some things about how settings work, and how we might want to support Settings in the plugin.

  1. The settingsDir and rootDir appear to always have the same value when Gradle starts executing the contents of settings.gradle. You can change the value of rootDir inside the script, but by that time, Gradle has already set the settings properties using only the gradle.properties in the settingsDir, the gradle.properties in the user's home directory, environment variables, system properties, and command line arguments. Gradle does not look at the gradle.properties in the rootDir because at the time it is evaluating the files, rootDir and settingsDir are the same. For this reason, I don't think we should evaluate anything in rootDir either.
  2. Gradle does not share any of the Settings properties with the projects it goes on to build. If users want to use a Settings property in a project's build.gradle, they need to set a Gradle.ext property in settings.gradle. They can then access this Gradle.ext property in the build.gradle file. This raises some interesting questions: What if the propertiesPluginEnvironmentNameProperty used by the settings is different than the one that gets used for the projects? What if the user sets the propertiesPluginEnvironmentNameProperty in the Gradle object? Should the plugin set the Gradle.ext property to try and have projects use the value from the Settings?

I don't think these questions have simple answers, but my initial thought is that the less we behave differently from Gradle's standard way, the better. I think that since a Setting and a Project are completely different by default, we probably shouldn't try to tie them together in the plugin.

I'm still trying to figure out how to get an instance of the Settings object in a unit test...

Vampire commented 10 years ago

I think the part I was missing was the fact that settings and project instances are completely separate. Applying the plugin in settings.gradle has nothing to do with projects or their properties, and only set properties to be used in settings.gradle itself. It is almost as if we have 2 plugins: a project properties plugin and a settings properties plugin, they just happen to run with the same code.

Exactly, now you got it. :-)

The settingsDir and rootDir appear to always have the same value when Gradle starts executing the contents of settings.gradle.

Exactly

You can change the value of rootDir inside the script, but by that time, Gradle has already set the settings properties using only the gradle.properties in the settingsDir, the gradle.properties in the user's home directory, environment variables, system properties, and command line arguments. Gradle does not look at the gradle.properties in the rootDir because at the time it is evaluating the files, rootDir and settingsDir are the same.

Exactly

For this reason, I don't think we should evaluate anything in rootDir either.

Hm, this is definitely a point. What I was thinking is, if I have a property that is used during settings.gradle time and during build.gradle time like in my case here, I'd like to be able to set it in one place, which would be the root project directory. For me personally it is not too important as your settings.gradle file lies beneath the root project build.gradle file. But I'd thought it would be a handy addition by the plugin.

... I don't think these questions have simple answers, but my initial thought is that the less we behave differently from Gradle's standard way, the better. I think that since a Setting and a Project are completely different by default, we probably shouldn't try to tie them together in the plugin.

Yep, I also think keeping them separate as Gradle does is the right think. The rootProjectDir-propertyfiles evaluation was just meant as convenient way to set properties that are the same for both, the build.gradle and the settings.gradle file.

I like to be able to define my properties for a build in one location. But to be honest, I guess it is veeeeeery uncommon to have root project dire non-equal to settings dir. Especially as I'm under the impression that it does not even work to change the root project directory from the settings.gradle file (http://forums.gradle.org/gradle/topics/changing_rootproject_projectdir_from_settings_gradle_does_not_work).

The question is just whether this mandates for parsing rootProjectDir-property-files or against.

I'm still trying to figure out how to get an instance of the Settings object in a unit test...

As I said, I didn't find a proper way like ProjectBuilder easily. But as the property parsing and overriding mechanism is the same and only the list of files differ, I personally could live with just having Unit Tests that test against the project. :-)

stevesaliman commented 10 years ago

There is one way you can set the property in one place.

While the Settings and Project objects are completely separate with separate properties, they share a Gradle instance. This means that if you put gradle.ext.somVar='someValue' in settings.gradle, you can use gradle.ext.someVar in your build.gradle file. Also, If you apply the plugin to both the settings and the projects, it would find properties from gradle.properties in the settingsDir (since it is also the root project dir).

I'm curious about the inability to change the root directory, however. I was able to change it in my sandbox, though in my case I had a flat layout instead of a hierarchical one - I wonder if that makes a difference...

I want to have unit tests to prove that we process the correct files when we apply to Settings because they are not the same files as we use for a Project. In the end, we may be able to just say that applying the plugin to Settings is "incubating" to avoid having to test it :-)

Vampire commented 10 years ago

While the Settings and Project objects are completely separate with separate properties, they share a Gradle instance. This means that if you put gradle.ext.somVar='someValue' in settings.gradle, you can use gradle.ext.someVar in your build.gradle file.

Yeah, you can also make sure that the root project dir and settings dir are not different and apply the plugin to Project and Settings. But this does not work if you change the root project dir in settings.xml before applying the plugin if rootProjectDir property files are not considered. Yes, using gradle.ext would also be a work-around. It is just probably more uncommon. But as there are at least 2 work-arounds and I don't need it personally yet, I'd be ok with not considering rootProjectDir property files. :-)

Also, If you apply the plugin to both the settings and the projects, it would find properties from gradle.properties in the settingsDir (since it is also the root project dir).

Not if you change the root project dir in settings.gradle, when this works finally.

I'm curious about the inability to change the root directory, however. I was able to change it in my sandbox, though in my case I had a flat layout instead of a hierarchical one - I wonder if that makes a difference...

Are you sure it did work? Could you give me your sandbox example where you think that it worked? I tried with a flat directory layout also and it did not work.

I want to have unit tests to prove that we process the correct files when we apply to Settings because they are not the same files as we use for a Project.

Yeah, sure, I absolutely agree. I just didn't see an easy way to do it. You could use some mocking framework, or create a subclass of Settings maybe. Instantiating the DefaultSettings without mocks is probably not too practicable as it needs some parameters.

In the end, we may be able to just say that applying the plugin to Settings is "incubating" to avoid having to test it :-)

Yes, this would also be possible. :-D

stevesaliman commented 10 years ago

I can put up my working rootDir change when I get home this evening.

In the meantime, it looks like you've got Cygwin on your box. I've seen Gradle work a little differently when Cygwin is involved.

What happens when you run the build from a DOS prompt instead of a Cygwin terminal?

My working example is running on a Linux box, but I'll try it from a Windows box with Cygwin to see if it still works.

Vampire commented 10 years ago

Nope, same behaviour in cygwin, dos box and on linux

stevesaliman commented 10 years ago

I've uploaded my rootDir-changing sandbox to https://github.com/stevesaliman/gradle-playground

If you run gradle show from the brother directory, it prints values for the settingsDir and the rootDir both before and after changing the rootProject's directory. It works under linux and cygwin. You can also run it from the master directory. There will be build failure, but the settings will still be printed.

Before running it, you'll probably need to build the gradle-properties plugin as version 1.3.2-SNAPSHOT.

Vampire commented 10 years ago

No it did not work, see the PR. ;-)

Vampire commented 10 years ago

So, how about this? :-)

stevesaliman commented 10 years ago

I've merged in this feature, but I'm going to remove the rootDir file checks for now.

When changing the rootDir works in gradle, we can determine how, if at all, we handle files in rootDir.

stevesaliman commented 10 years ago

I've uploaded the latest snapshot release to maven central. The only outstanding issue we have is #8. If we can live without it, I can do a release tomorrow or Tuesday.

I've changed the version number to 1.4.0 because I think that adding the ability to apply the plugin in settings.gradle is a big enough deal to justify bumping the minor number.

Vampire commented 10 years ago

I've merged in this feature, but I'm going to remove the rootDir file checks for now.

Ok, but I would replace

files.add(new PropertyFile("${settings.rootDir}/gradle.properties", FileType.OPTIONAL)) files.add(new PropertyFile("${settings.rootDir}/gradle-${envName}.properties", FileType.ENVIRONMENT))

by

files.add(new PropertyFile("${settings.settingsDir}/gradle.properties", FileType.OPTIONAL)) files.add(new PropertyFile("${settings.settingsDir}/gradle-${envName}.properties", FileType.ENVIRONMENT))

as this more resembles what Gradle is doing. Otherwise you are looking into the root project dir and not in the settings dir if the root project dir got changed before the plugin is applied.

When changing the rootDir works in gradle, we can determine how, if at all, we handle files in rootDir.

This could work already, I fixed the issue and submitted a PR to Gradle. It was not yet applied though. Actually it does work already as long as you execute gradlew from the root project dir and not from the settings dir. So if you want to try out things, you can, even without my fix.

As noted earlier, Gradle itself only looks into the settings dir and gradle user home. So if you want to stay closer to what Gradle does, the current variant (with my fix from above) would be it. Adding the root project dir properties just would be a convenience addition by the plugin, but as there is a work-around we discussed earlier and I don't need it personally, I could live with it this way. :-)

I've uploaded the latest snapshot release to maven central. The only outstanding issue we have is #8. If we can live without it, I can do a release tomorrow or Tuesday.

At least I could live with it coming in 1.4.1 or similar (or not) as it is just an additional safety net and I'd like to use an official release version with the remaining of my changes. :-) So from my point of view, by any means do a release as soon as you can with the change noted above, thanks. :-)

I've changed the version number to 1.4.0 because I think that adding the ability to apply the plugin in settings.gradle is a big enough deal to justify bumping the minor number.

Definitely. My PRs didn't include any versioning changes as I didn't want to hook into your versioning policy. :-)

Vampire commented 10 years ago

Hey there, just wanting to check back whether you have read the first part of my last comment, as you released 1.4.0 without that change. :-)

stevesaliman commented 10 years ago

Now you know why I like to have unit tests :-)

I did indeed miss that bit. I'm releasing 1.4.1 now with that change.

Vampire commented 10 years ago

No problem, the feature is incubating anyway. :-D