Skip to content

Skip git invocation if properties are already computed #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Skip git invocation if properties are already computed #411

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions src/main/java/pl/project13/maven/git/GitCommitIdMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import com.google.common.io.Files;
import java.io.OutputStream;


import pl.project13.maven.git.build.BuildServerDataProvider;
import pl.project13.maven.git.log.LoggerBridge;
import pl.project13.maven.git.log.MavenLoggerBridge;
Expand All @@ -63,6 +64,8 @@
*/
@Mojo(name = "revision", defaultPhase = LifecyclePhase.INITIALIZE, threadSafe = true)
public class GitCommitIdMojo extends AbstractMojo {
private static final String CONTEXT_KEY = GitCommitIdMojo.class.getName() + ".properties";

/**
* The Maven Project.
*/
Expand Down Expand Up @@ -324,7 +327,7 @@ public class GitCommitIdMojo extends AbstractMojo {
*/
@Parameter(defaultValue = "30000")
long nativeGitTimeoutInMs;

/**
* Use branch name from build environment. Set to {@code 'false'} to use JGit/GIT to get current branch name.
* Useful when using the JGitflow maven plugin.
Expand All @@ -333,7 +336,7 @@ public class GitCommitIdMojo extends AbstractMojo {
*/
@Parameter(defaultValue = "true")
boolean useBranchNameFromBuildEnvironment;

/**
* Injected {@link BuildContext} to recognize incremental builds.
*/
Expand Down Expand Up @@ -432,20 +435,31 @@ public void execute() throws MojoExecutionException {
String trimmedPrefix = prefix.trim();
prefixDot = trimmedPrefix.equals("") ? "" : trimmedPrefix + ".";

loadGitData(properties);
loadBuildData(properties);
propertiesReplacer.performReplacement(properties, replacementProperties);
propertiesFilterer.filter(properties, includeOnlyProperties, this.prefixDot);
propertiesFilterer.filterNot(properties, excludeProperties, this.prefixDot);
// check if properties have already been injected
Properties contextProperties = getContextProperties(project);
boolean alreadyInjected = injectAllReactorProjects && contextProperties != null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (alreadyInjected) {
log.info("injectAllReactorProjects is enabled and this project already contains properties");
properties = contextProperties;
} else {
loadGitData(properties);
loadBuildData(properties);
propertiesReplacer.performReplacement(properties, replacementProperties);
propertiesFilterer.filter(properties, includeOnlyProperties, this.prefixDot);
propertiesFilterer.filterNot(properties, excludeProperties, this.prefixDot);
}
logProperties();

if (generateGitPropertiesFile) {
maybeGeneratePropertiesFile(properties, project.getBasedir(), generateGitPropertiesFilename);
}
publishPropertiesInto(project);

if (injectAllReactorProjects) {
appendPropertiesToReactorProjects();
if (!alreadyInjected) {
publishPropertiesInto(project);

if (injectAllReactorProjects) {
appendPropertiesToReactorProjects();
}
}
} catch (Exception e) {
handlePluginFailure(e);
Expand Down Expand Up @@ -492,6 +506,7 @@ private void appendPropertiesToReactorProjects() {
log.info("{}] project {}", mavenProject.getName(), mavenProject.getName());

publishPropertiesInto(mavenProject);
mavenProject.setContextValue(CONTEXT_KEY, properties);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

}
}

Expand Down Expand Up @@ -609,11 +624,11 @@ private void maybeGeneratePropertiesFile(@Nonnull Properties localProperties, Fi
} catch (final IOException ex) {
throw new RuntimeException("Cannot create custom git properties file: " + gitPropsFile, ex);
}

if (buildContext != null) {
buildContext.refresh(gitPropsFile);
}

} else {
log.info("Properties file [{}] is up-to-date (for module {})...", gitPropsFile.getAbsolutePath(), project.getName());
}
Expand Down Expand Up @@ -678,6 +693,10 @@ private Properties readProperties(@Nonnull File propertiesFile) throws CannotRea
}
}

private Properties getContextProperties(MavenProject project) {
return (Properties) project.getContextValue(CONTEXT_KEY);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

}

static class CannotReadFileException extends Exception {
private static final long serialVersionUID = -6290782570018307756L;

Expand Down