Skip to content
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.

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

Allow property overwrites for version fields, outfile and jar analogous to the ANT task #49

Merged
merged 2 commits into from
Dec 19, 2016

Conversation

tofi86
Copy link
Contributor

@tofi86 tofi86 commented Dec 11, 2016

This PR fixes my Feature Request #48

The changes from this PR allow overwriting the version fields, <outfile> and <jar> analogous to the ANT task when using an external configuration file with <infile>.

Example:

<configuration>
	<infile>${project.build.sourceDir}/launch4j-config.xml</infile>

	<jar>${project.build.directory}/${my.finalJarName}.jar</jar>
	<outfile>${project.build.directory}/${my.finalShortName}.exe</outfile>
	<versionInfo>
		<fileVersion>${my.versionNumber.withBuildId}</fileVersion>
		<txtFileVersion>${my.versionNumber}</txtFileVersion>
		<productVersion>${my.versionNumber.withBuildId}</productVersion>
		<txtProductVersion>${my.versionNumber}</txtProductVersion>
	</versionInfo>
</configuration>

Overwrites are logged in the maven debug log.

Important note:
To be able to debug these code changes, I had to fix the debug logging method printState() so it also takes properties from the external configuration file into account.
This has been missing in the original PR #32 by @adamdec

It’s possible to overwrite the properties 'jar', 'outfile', 'fileVersion', 'txtFileVersion', 'productVersion', 'txtProductVersion'

Example:
<configuration>
	<infile>${project.build.sourceDir}/launch4j-config.xml</infile>

	<jar>${project.build.directory}/${my.finalJarName}.jar</jar>
	<outfile>${project.build.directory}/${my.finalShortName}.exe</outfile>
	<versionInfo>
		<fileVersion>${my.versionNumber.withBuildId}</fileVersion>
		<txtFileVersion>${my.versionNumber}</txtFileVersion>
		<productVersion>${my.versionNumber.withBuildId}</productVersion>
		<txtProductVersion>${my.versionNumber}</txtProductVersion>
	</versionInfo>
</configuration>

For ANT task analogy see https://sourceforge.net/p/launch4j/git/ci/master/tree/src/net/sf/launch4j/ant/Launch4jTask.java#l84
@lukaszlenart
Copy link
Collaborator

lukaszlenart commented Dec 13, 2016

I would revert a bit logic here:

  • first try to load config file
  • if loaded, override properties if property isn't null
  • if not loaded, put out properties as now

There can be one global variable config for method execute and additional flag to mark if config was loaded from a file (to allow override).

With such approach you can override any property you want, instead re-defining put logic as now for versionInfo.

wdyt?

@tofi86
Copy link
Contributor Author

tofi86 commented Dec 15, 2016

thanks for your feedback @lukaszlenart!

I was trying to stick to the ANT logic as the Launch4jMojo class is obviously based on that...

Sure, we could provide the option to override ANY property, but not even launch4j provides this functionality.

The problem I encountered here was, that several properties will never(!) evaluate to null because they have a maven mojo default value. That's why I evaluate the jar and outfile default values first
(see lines 312, 218 -> https://github.com/lukaszlenart/launch4j-maven-plugin/pull/49/files#diff-40b1a260cdec12953866889e3827e7e3R312 ) before beeing able to check whether to overwrite or not...

Since there are several other properties with default values one would need to check every of those for the default value at first:

  • property is not null
  • is the property value the default value or a custom provided value

Checking whether a value is the default value can eventually be pretty hard, as the default value is not stored in some maven configuration, accessible by Java. For jar and outfile I was lucky to implement this, because the default value can be programmatically built from maven default properties:

@Parameter(defaultValue = "${project.build.directory}/${project.artifactId}.exe")
private File outfile;
...
@Parameter(defaultValue = "${project.build.directory}/${project.build.finalName}.jar")
private String jar;
...
String jarDefaultValue = project.getBuild().getDirectory() + "/" + project.getBuild().getFinalName() + ".jar";
File outFileDefaultValue = new File(project.getBuild().getDirectory() + "/" + project.getArtifactId() + ".exe");

I find it hard to describe this behaviour, but I hope you get what I mean... ;-)

@lukaszlenart
Copy link
Collaborator

Hm... so maybe it would be good to add a flag <override>true|false</override> and then override settings from config file?

@tofi86
Copy link
Contributor Author

tofi86 commented Dec 17, 2016

Hm... so maybe it would be good to add a flag true|false and then override settings from config file?

Nice idea, but imagine the following configuration:

<configuration>
    <infile>path/to/launch4j-config.xml</infile>
    <override>true</override>
    <outfile>path/to/outfile.exe</outfile>
</configuration>

You would expect that only the outfile property get's overwritten after the initial import of infile.
However, also the jarfile property gets overwritten — plus some other properties with predefined default values — as they doesn't equal to null.

Just checking for override = true and property != null does not work:

if(override = true && property != null) {
    config.property = property;
}

property could never be null if it has a maven default value. And we have no chance to find out whether the not-null value is a maven default value or a user configuration property...

@lukaszlenart
Copy link
Collaborator

I see your point but this can be simply fixed by using setters - anyway, let's start with this and then we can always refactor to much more proper solution :)

@lukaszlenart lukaszlenart merged commit 48b021f into orphan-oss:master Dec 19, 2016
@tofi86
Copy link
Contributor Author

tofi86 commented Dec 19, 2016

Okay, great!

I'm not a maven pro so I was probably a bit uncertain how to resolve that... If there's methods for that, sure, let's refactor that in a next step... :)

Thanks for your review and thanks for the merge!

tofi86 added a commit to paginagmbh/EPUB-Checker that referenced this pull request Dec 29, 2016
…cific properties

possible since 1.7.15 through changes committet in orphan-oss/launch4j-maven-plugin#49
@lukaszlenart
Copy link
Collaborator

PRs are welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants