Skip git invocation if properties are already computed #411
Conversation
In a multi-modules project, one can set injectAllReactorProjects to true to add all the properties to each reactor's project, but there's no automated effort to skip git execution if properties are already set unless runOnlyOnce is set to true. But runOnce has two issues itself: - it assumes that the top-level project is included in the reactor - it skips the whole plugin execution To address this, detect during plugin execution if properties from a previous project in the reactor have been already computed and added to the current project's context, and skip git execution if this is the case.
| propertiesFilterer.filterNot(properties, excludeProperties, this.prefixDot); | ||
| // check if properties have already been injected | ||
| Properties contextProperties = getContextProperties(project); | ||
| boolean alreadyInjected = injectAllReactorProjects && contextProperties != null; |
There was a problem hiding this comment.
This is indeed an interesting approach, but I fear the check if the data is already injected is not that easy :-/
For example one could configure this plugin with different (multiple) repositories and generate a different set of properties with a different prefix for each repository (see here for a full example).
I think for our case on top of the different prefix, we also might need to be concerned in what phase the plugin was executed. An execution in a later phase would (and prop should) overwrite the existing properties (the question then would be what do we really expect from this overwrite?....not sure...not even sure if the plugin has an easy way to check in what phase it was executed :/
Furthermore what happens if a project configures different settings like wants to exclude different properties? I think in our current execution what would happen is that project A generates the properties, applies the filters and happens to have the right properties that matches the configuration of project A. When project B comes, it regenerates the properties (wasted effort) and then applies his own filter and then happens to have the properties that matches project B. I guess from my perspective the 'filtering' should not be the part of what will be cached. The time it takes to filter should be trivial. What consumes time is 'gathering' the properties and hence those should be the ones cached. Just to note that we really need to be careful here, since we don't want to expose properties that are 'excluded' as per project configuration (not just in the generated file, but also what is accessible via maven filtering).
Also not sure how maven would treat when we use multiple threads. Is this in general thread save? Is the project that wins the 'thread' race always be the same? I guess that might be out of scope too.
| @@ -492,6 +506,7 @@ private void appendPropertiesToReactorProjects() { | |||
| log.info("{}] project {}", mavenProject.getName(), mavenProject.getName()); | |||
|
|
|||
| publishPropertiesInto(mavenProject); | |||
| mavenProject.setContextValue(CONTEXT_KEY, properties); | |||
There was a problem hiding this comment.
Excuse my ignorance, but do you happen to know what a ContextValue is?
The relevant javadoc states that context values are intended to allow core extensions to associate derived state with project instances. To be honest, even after reading that multiple times, I still don't understand if this plugin can use them (aka. Is this plugin considered a core extensions?) and what values this plugin would be allowed to store in them (what does associate derived state with project instances even mean?? Properties that can be derived from other project properties?).
Why not store all properties as single property? When a user wants to access them, well whatever that's up to the user. User could change the properties one-by-one anyways when wanted....
| @@ -678,6 +693,10 @@ private Properties readProperties(@Nonnull File propertiesFile) throws CannotRea | |||
| } | |||
| } | |||
|
|
|||
| private Properties getContextProperties(MavenProject project) { | |||
| return (Properties) project.getContextValue(CONTEXT_KEY); | |||
There was a problem hiding this comment.
Mhh I would like to avoid class cast exceptions. Could you check if the stored type is at least from type Properties?
e.g.
Object stored = project.getContextValue(CONTEXT_KEY);
if (stored instanceof Properties) {
return (Properties)stored;
}
return null;
…skip for reactor projects with injectAllReactorProjects=true also works properly when a user specifies multiple execution with a different prefix
In a multi-modules project, one can set injectAllReactorProjects to
true to add all the properties to each reactor's project, but there's
no automated effort to skip git execution if properties are already set
unless runOnlyOnce is set to true.
But runOnce has two issues itself:
To address this, detect during plugin execution if properties from a
previous project in the reactor have been already computed and added
to the current project's context, and skip git execution if this is the
case.
Context
Contributor Checklist
mvn clean packagecheckstylecoding style definition:mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B