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

PropertiesFileTransformer breaks Reproducible builds in 8.1.1 #856

Closed
agascon opened this issue Apr 13, 2023 · 9 comments · Fixed by #876
Closed

PropertiesFileTransformer breaks Reproducible builds in 8.1.1 #856

agascon opened this issue Apr 13, 2023 · 9 comments · Fixed by #876

Comments

@agascon
Copy link
Contributor

agascon commented Apr 13, 2023

Shadow Version

8.1.1

Gradle Version

Not relevant.

Expected Behavior

PropertiesFileTransformer supports having reproducible builds by not introducing any timestamp dependant of the build time.

Actual Behavior

Fix for supporting reproducible builds introduced in e46bd20 in 8.0.0 were overridden by 9bffd1e in 8.1.1

PropertiesFileTransformer is back to use Properties class and it's introducing timestamps into transformed files.

@johnrengelman
Copy link
Collaborator

@agascon do you have something to demonstrates the issue? It doesn't appear that any of the changes were actually overridden in the merge (https://github.com/johnrengelman/shadow/blob/main/src/main/groovy/com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer.groovy#L185-L203). It was just 2 changes that were in the same place, but the 3-way merge accounted for it. Additionally, the tests added in e46bd204917bc004c1a92c7d1ffa36250b192904https://github.com/johnrengelman/shadow/commit/e46bd204917bc004c1a92c7d1ffa36250b192904 are still passing.

@agascon
Copy link
Contributor Author

agascon commented Apr 13, 2023

Unfortunately the project where I detected this is private but will try to provide something.

I understand problem is in https://github.com/johnrengelman/shadow/blob/main/src/main/groovy/com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer.groovy#L253. That store method is not the one overridden in CleanProperties.

@agascon
Copy link
Contributor Author

agascon commented Apr 13, 2023

I created an Spring Boot dummy application and setup shadow in build script.

When using version 8.1.0 and between 2 separate builds...

❯ gradle clean build
executing gradlew instead of gradle

BUILD SUCCESSFUL in 17s
9 actionable tasks: 9 executed
❯ md5sum build/libs/demo-0.0.1-SNAPSHOT-all.jar
bfe7760f6f6dcc464002ce6b6134c406  build/libs/demo-0.0.1-SNAPSHOT-all.jar
❯ gradle clean build
executing gradlew instead of gradle

BUILD SUCCESSFUL in 5s
9 actionable tasks: 9 executed
❯ md5sum build/libs/demo-0.0.1-SNAPSHOT-all.jar
bfe7760f6f6dcc464002ce6b6134c406  build/libs/demo-0.0.1-SNAPSHOT-all.jar

However, when using 8.1.1...

❯ gradle clean build
executing gradlew instead of gradle

BUILD SUCCESSFUL in 14s
9 actionable tasks: 9 executed
❯ md5sum build/libs/demo-0.0.1-SNAPSHOT-all.jar
f292927c37b4f90014ae8eb11e9a2743  build/libs/demo-0.0.1-SNAPSHOT-all.jar
❯ gradle clean build
executing gradlew instead of gradle

BUILD SUCCESSFUL in 6s
9 actionable tasks: 9 executed
❯ md5sum build/libs/demo-0.0.1-SNAPSHOT-all.jar
10b8c61cfefe1531ad711719c5aa8315  build/libs/demo-0.0.1-SNAPSHOT-all.jar

And in this last case...

❯ unzip -q -c build/libs/demo-0.0.1-SNAPSHOT-all.jar META-INF/spring.factories
#
#Thu Apr 13 22:35:48 CEST 2023
org.springframework.boot.autoconfigure.AutoConfigurationImportListener=org.springframework.boot.autoconfigure.condition.ConditionEvaluationReportAutoConfigurationImportListener
org.springframework.boot.diagnostics.FailureAnalysisReporter=org.springframework.boot.diagnostics.LoggingFailureAnalysisReporter
<truncated>

@agascon
Copy link
Contributor Author

agascon commented Apr 13, 2023

build.gradle

import com.github.jengelman.gradle.plugins.shadow.transformers.PropertiesFileTransformer

plugins {
	id 'java'
	id 'org.springframework.boot' version '2.7.10'
	id 'io.spring.dependency-management' version '1.0.15.RELEASE'
	id 'com.github.johnrengelman.shadow' version '8.1.1'
}

group = 'com.example'
version = '0.0.1-SNAPSHOT'
sourceCompatibility = '17'

repositories {
	mavenCentral()
}

dependencies {
	implementation 'org.springframework.boot:spring-boot-starter'
	testImplementation 'org.springframework.boot:spring-boot-starter-test'
}

tasks.named('test') {
	useJUnitPlatform()
}

tasks {
	shadowJar {
		preserveFileTimestamps false
		reproducibleFileOrder true

		// Required for Spring
		mergeServiceFiles()
		append 'META-INF/spring.handlers'
		append 'META-INF/spring.schemas'
		append 'META-INF/spring.tooling'
		append 'META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports'
		append 'META-INF/spring/org.springframework.boot.actuate.autoconfigure.web.ManagementContextConfiguration.imports'
		transform(PropertiesFileTransformer) {
			paths = ['META-INF/spring.factories']
			mergeStrategy = "append"
		}
	}
}

build.dependsOn shadowJar

@agascon
Copy link
Contributor Author

agascon commented Apr 13, 2023

In any case, I have clear where the problem is, if had time will try to raise a PR with the fix

@johnrengelman
Copy link
Collaborator

Thanks! I think we just need a test to trigger this. I’m not sure why the overridden method wouldn’t be getting invoked. Or why the existing tests wouldn’t be failing. If you beat me to getting a test or fix into a PR, awesome. I may have some time to get dog into it this weekend.

@agascon
Copy link
Contributor Author

agascon commented Apr 18, 2023

Raised a PR, please check...
#858

@facewindu
Copy link

I'm facing this issue too. I see the PR above from more than a year ago. Any chance this gets some traction?

@agascon
Copy link
Contributor Author

agascon commented May 27, 2024

Looks @johnrengelman is looking to transfer the project to new maintainers.

See this discussion...

If there is a new repo when this PR can be pushed, I'm also interesting on having this fixed.

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 a pull request may close this issue.

3 participants