Skip to content

Conversation

@snazy
Copy link
Contributor

@snazy snazy commented Nov 17, 2025

PropertiesFileTransformer leverages java.util.Properties, which relies on java.util.Hashtable. The serialized properties are not guaranteed to be reproducible.

This change changes the transformer to use a sorted map to generate reproducible output. The existing class CleanProperties is replaced with ReproducibleProperties.
Functionality around charset handling is retained, and extended with a functionality to generate unicode escapes (ASCII output).

Refs #1848.


  • CHANGELOG's "Unreleased" section has been updated, if applicable.

@snazy
Copy link
Contributor Author

snazy commented Nov 17, 2025

Depends on #1856

@snazy snazy marked this pull request as ready for review November 18, 2025 08:50
`PropertiesFileTransformer` leverages `java.util.Properties`, which relies on `java.util.Hashtable`. The serialized properties are not guaranteed to be reproducible.

This change changes the transformer to use a sorted map to generate reproducible output. The existing class `CleanProperties` is replaced with `ReproducibleProperties`.
Functionality around charset handling is retained, and extended with a functionality to generate unicode escapes (ASCII output).
@snazy snazy force-pushed the pft-reproducible branch 2 times, most recently from 30d2084 to f7c0b62 Compare November 18, 2025 13:44
@snazy
Copy link
Contributor Author

snazy commented Nov 18, 2025

Okay, GH is broken :(

@Goooler
Copy link
Member

Goooler commented Nov 18, 2025

Let's wait to tomorrow 😁

@snazy
Copy link
Contributor Author

snazy commented Nov 18, 2025

Oh, it's the "zulu" installer that barks.

@Goooler Goooler force-pushed the pft-reproducible branch 2 times, most recently from f53e3ed to 8871cd9 Compare November 19, 2025 01:50
@Goooler Goooler changed the title PropertiesFileTransformer - make merged properties reproducible Make the output of PropertiesFileTransformer reproducible Nov 19, 2025
@Goooler Goooler requested a review from Copilot November 19, 2025 02:12
Copilot finished reviewing on behalf of Goooler November 19, 2025 02:15

This comment was marked as outdated.

}.toString().reader().buffered()

private val String?.couldBeCommentWithTimestamp: Boolean get() {
return this != null && startsWith("#") && length == lengthOfExpectedTimestamp
Copy link
Member

Choose a reason for hiding this comment

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

The old logic is useful for removing date comments only. All comments are removed in this PR. See Goooler#102 (comment).

I'm considering whether we should introduce a new property to opt for the feature in. The new flag will control:

  1. Remove all comments or not.
  2. Sort all properties or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure a flag's worth it.
The comment was always empty (via this).
Sorting vs not-sorting doesn't change the contents.

But we could (or should?) add a comment saying something like

# Generated by GradleUp shadow plugin from one or more input properties files.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

The comment was always empty

You are right!

add a new comment...

Let's keep the current behavior; otherwise, we need to add comments for the property-related transformers.

Copy link
Member

Choose a reason for hiding this comment

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

Updated a test for comments, see #1868.

@Goooler Goooler merged commit 90b0b3e into GradleUp:main Nov 19, 2025
13 checks passed
@Goooler
Copy link
Member

Goooler commented Nov 19, 2025

I reverted the name change of CleanProperties to reduce the diff in this PR. You can send a new one for the renaming.

@Goooler
Copy link
Member

Goooler commented Nov 19, 2025

Addressing the renaming to #1869.

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