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

buildSrc project support #28

Open grv87 opened 6 years ago

grv87 commented 6 years ago

There is a generally common task to provide the same Gradle properties to buildSrc project. See e.g. this discussion. I see that this plugin could help with that.

I tried to use it from buildSrc and found two main problems:

  1. Plugin resolves environmentFileDir as subdirectory of projectDir, here: https://github.com/stevesaliman/gradle-properties-plugin/blob/c04a93cf0418732297271164759547f308dfab69/src/main/groovy/net/saliman/gradle/plugin/properties/PropertiesPlugin.groovy#L253

    This is not acceptable for buildSrc project. I could pass ORG_GRADLE_PROJECT_environmentFileDir environment variable, but it should be absolute path, the same for the main project and buildSrc.

  2. Plugin allows to customize location of gradle-${environmentName}.properties only. For buildSrc project gradle.properties of the main project should be loaded too. I would like to add this feature. It could be turned on for buildSrc projects by default or manually. What would you accept?

stevesaliman commented 6 years ago

Thank you for offering to help out, contributions are always appreciated.

Line 253 is part of a loop that is walking up the project structure to load environment files, first from the project directory, then the parent project.

I think the buildSrc directory should be treated like any other sub-project, which means 2 things:

  1. The properties plugin should only effect things in the buildSrc project if it is explicitly applied to the buildSrc project via its own build.gradle file.
  2. If the properties plugin is applied to the buildSrc project, it should process files like it would with any other sub-project.

The issue with the code at line 253 is that the buildSrc project doesn't have a parent project, so the plugin doesn't move up a directory like it would with a typical sub-project.

The solution is probably to create a buildPropertyFileListFromBuildSrc method that gets called for buildSrc projects that works just like buildPropertyFileListFromProject, except that it doesn't have the loop, and it uses the parent directory of the project directory to process the main project's files after processing the buildSrc files themselves.

I can take a look at it, but it might be a couple of weeks until I can get to it.

grv87 commented 6 years ago

Even for single project or sub-projects, ORG_GRADLE_PROJECT_environmentFileDir could be absolute path, and the code in line 253 would not work. If this is strict requirement it should be stated so in README. But I see that it could be easily changed.

stevesaliman commented 6 years ago

I took another look at how properties get set in a buildSrc project without any plugins. Gradle doesn't normally pass any properties from the parent project into a buildSrc project, so I don't think the plugin should do it by default either.

But I do see how this could be a useful feature of the properties plugin. I think I'm going to create a configuration closure, similar to other plugins, that we can use to tell the plugin that we want inheritance into buildSrc projects. I've started working on a solution, and I'll update the plugin when I can.

stevesaliman commented 6 years ago

This may be trickier than I thought. Gradle currently has a bug where properties set on the command line are not passed into the buildSrc project as command line arguments. This means that as soon as you apply the properties plugin to the buildSrc directory, you lose any properties that were set with a -P option from the command line.

I'm still looking for a workaround.

grv87 commented 6 years ago

I don't think that missing command line properties are a big problem. First of all, I'm not sure this is a bug. Do you have a link to an issue report?

Second of all, personally, I don't use command line properties at all. I use either files (locally and in tests) or environment variables (under CI).

I think this could be a known limitation.

grv87 commented 6 years ago

I started working on the implementation too.

If this is not the default behavior, how exactly do you see this could be turned on? Right now all files are read on plugin application. If we use configuration closure, this wouldn't work.

If I understand correctly, the plugin doesn't replace Gradle's own method of reading properties, it just reads properties in its own way on top of Gradle's. If that's correct, user could pass special property by standard way (i.e. gradle.properties) and we could use it during plugin application to turn on new behavior.

stevesaliman commented 6 years ago

The issue is mentioned in https://discuss.gradle.org/t/pass-properties-to-buildsrc-build-part/7032 where one of the Gradle developers specifically mention that -P doesn't work for buildSrc projects, but I didn't see an actual issue for this. Maybe we should create one to see if they can add a way to get startparameters from within a buildSrc project.

I appreciate that you don't use command line properties, but there are a lot of people out there who do - especially when there are credentials involved and they don't want to store them in files. Currently, properties are processed by the parent project, and the value does appear to be inherited by the buildSrc project, but this behavior breaks when the properties plugin is applied to the buildSrc project.

This is because of how the plugin works. You are correct in that the plugin does not replace Gradle's own method of reading properties. It reproduces it after the fact by doing what Gradle would do, in the same order, but with a couple of extra steps thrown in. The plugin can only do this if it has access to the command line properties in some way, which it doesn't have in the case of buildSrc projects. I think most users would expect the plugin to behave the same everywhere it is applied, and I'm trying to weigh the negatives of having the command line disabled in buildSrc projects against the inconvenience of not being able to apply it to buildSrc projects.

As for configuration, it would be nice to use configuration closures like other plugins, but this one may have to be different because unlike other plugins, it does its work before the execution phase. I'd have to see if there is a way around that.

grv87 commented 6 years ago

I finally understood what you meant. Project properties passed through command line are available in buildSrc scripts. But they are not available in gradle.startParameter.projectProperties and so plugin can't do its work. So, if we turn on the plugin then command line properties would stop to work (or, more accurately, properties provided in command line would be overridden from other sources. But properties passed through command line only and not overridden anywhere else would still be available). I agree, this would be strange behavior and hard to explain and understand.

First I thought that command line properties are not available at all - then the new feature would not break anything.

It's very strange since startParameter.projectProperties for buildSrc are set: https://github.com/gradle/gradle/blob/3af000eb3096732d10fba645ba1f54fb12e3c364/subprojects/core/src/main/java/org/gradle/initialization/buildsrc/BuildSourceBuilder.java#L87

stevesaliman commented 6 years ago

Sorry I wasn't as clear as I could be on the plugin issue :-)

I also noticed that line of code and was also surprised by it. I'll keep an eye on the latest releases of Gradle, to see if it starts working as we'd expect, and I may try to submit a patch to Gradle itself if I can figure out what needs to be done.