Copilot commented on code in PR #328:
URL: https://github.com/apache/maven-archiver/pull/328#discussion_r2636061943
##########
src/main/java/org/apache/maven/archiver/PomPropertiesUtil.java:
##########
@@ -51,44 +51,25 @@ private Properties loadPropertiesFile(File file) throws
IOException {
}
}
- private boolean sameContents(Properties props, File file) throws
IOException {
- if (!file.isFile()) {
- return false;
+ private void createPropertiesFile(Properties properties, Path outputFile)
throws IOException {
+ Path outputDir = outputFile.getParent();
+ if (outputDir != null) {
+ Files.createDirectories(outputDir);
}
-
- Properties fileProps = loadPropertiesFile(file);
- return fileProps.equals(props);
- }
-
- private void createPropertiesFile(Properties properties, File outputFile,
boolean forceCreation)
- throws IOException {
- File outputDir = outputFile.getParentFile();
- if (outputDir != null && !outputDir.isDirectory() &&
!outputDir.mkdirs()) {
- throw new IOException("Failed to create directory: " + outputDir);
- }
- if (!forceCreation && sameContents(properties, outputFile)) {
- return;
- }
-
- try (PrintWriter pw = new PrintWriter(outputFile, "ISO-8859-1");
- StringWriter sw = new StringWriter()) {
-
- properties.store(sw, null);
-
- List<String> lines = new ArrayList<>();
- try (BufferedReader r = new BufferedReader(new
StringReader(sw.toString()))) {
- String line;
- while ((line = r.readLine()) != null) {
- if (!line.startsWith("#")) {
- lines.add(line);
- }
- }
- }
-
- Collections.sort(lines);
- for (String l : lines) {
- pw.println(l);
- }
+ // For reproducible builds, sort the properties and drop comments.
+ // The java.util.Properties class doesn't guarantee order so we have
+ // to write the file using a Writer.
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ properties.store(baos, null);
+ // The encoding can be either UTF-8 or ISO-8859-1, as any non ascii
character
+ // is transformed into a \\uxxxx sequence anyway
+ String output = Arrays.stream(
+
baos.toString(StandardCharsets.ISO_8859_1.name()).split("\\r?\\n"))
Review Comment:
The baos.toString() method is called with a charset name string instead of
using the Charset object directly. This is a deprecated usage pattern. Consider
using baos.toString(StandardCharsets.ISO_8859_1) directly, which is available
since Java 10.
```suggestion
baos.toString(StandardCharsets.ISO_8859_1).split("\\r?\\n"))
```
##########
src/main/java/org/apache/maven/archiver/PomPropertiesUtil.java:
##########
@@ -51,44 +51,25 @@ private Properties loadPropertiesFile(File file) throws
IOException {
}
}
- private boolean sameContents(Properties props, File file) throws
IOException {
- if (!file.isFile()) {
- return false;
+ private void createPropertiesFile(Properties properties, Path outputFile)
throws IOException {
+ Path outputDir = outputFile.getParent();
+ if (outputDir != null) {
+ Files.createDirectories(outputDir);
}
-
- Properties fileProps = loadPropertiesFile(file);
- return fileProps.equals(props);
- }
-
- private void createPropertiesFile(Properties properties, File outputFile,
boolean forceCreation)
- throws IOException {
- File outputDir = outputFile.getParentFile();
- if (outputDir != null && !outputDir.isDirectory() &&
!outputDir.mkdirs()) {
- throw new IOException("Failed to create directory: " + outputDir);
- }
- if (!forceCreation && sameContents(properties, outputFile)) {
- return;
- }
-
- try (PrintWriter pw = new PrintWriter(outputFile, "ISO-8859-1");
- StringWriter sw = new StringWriter()) {
-
- properties.store(sw, null);
-
- List<String> lines = new ArrayList<>();
- try (BufferedReader r = new BufferedReader(new
StringReader(sw.toString()))) {
- String line;
- while ((line = r.readLine()) != null) {
- if (!line.startsWith("#")) {
- lines.add(line);
- }
- }
- }
-
- Collections.sort(lines);
- for (String l : lines) {
- pw.println(l);
- }
+ // For reproducible builds, sort the properties and drop comments.
+ // The java.util.Properties class doesn't guarantee order so we have
+ // to write the file using a Writer.
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ properties.store(baos, null);
+ // The encoding can be either UTF-8 or ISO-8859-1, as any non ascii
character
+ // is transformed into a \\uxxxx sequence anyway
+ String output = Arrays.stream(
+
baos.toString(StandardCharsets.ISO_8859_1.name()).split("\\r?\\n"))
Review Comment:
The Properties.store(OutputStream) method uses ISO-8859-1 encoding by
default, but this is not explicitly guaranteed by the API contract. If the
Properties implementation or JDK changes this behavior, the subsequent toString
with ISO_8859_1 may produce incorrect results. Consider using
Properties.store(Writer) directly with an ISO-8859-1 OutputStreamWriter to
ensure encoding consistency throughout the entire process.
##########
src/main/java/org/apache/maven/archiver/PomPropertiesUtil.java:
##########
@@ -51,44 +51,25 @@ private Properties loadPropertiesFile(File file) throws
IOException {
}
}
- private boolean sameContents(Properties props, File file) throws
IOException {
- if (!file.isFile()) {
- return false;
+ private void createPropertiesFile(Properties properties, Path outputFile)
throws IOException {
+ Path outputDir = outputFile.getParent();
+ if (outputDir != null) {
+ Files.createDirectories(outputDir);
}
-
- Properties fileProps = loadPropertiesFile(file);
- return fileProps.equals(props);
- }
-
- private void createPropertiesFile(Properties properties, File outputFile,
boolean forceCreation)
- throws IOException {
- File outputDir = outputFile.getParentFile();
- if (outputDir != null && !outputDir.isDirectory() &&
!outputDir.mkdirs()) {
- throw new IOException("Failed to create directory: " + outputDir);
- }
- if (!forceCreation && sameContents(properties, outputFile)) {
- return;
- }
-
- try (PrintWriter pw = new PrintWriter(outputFile, "ISO-8859-1");
- StringWriter sw = new StringWriter()) {
-
- properties.store(sw, null);
-
- List<String> lines = new ArrayList<>();
- try (BufferedReader r = new BufferedReader(new
StringReader(sw.toString()))) {
- String line;
- while ((line = r.readLine()) != null) {
- if (!line.startsWith("#")) {
- lines.add(line);
- }
- }
- }
-
- Collections.sort(lines);
- for (String l : lines) {
- pw.println(l);
- }
+ // For reproducible builds, sort the properties and drop comments.
+ // The java.util.Properties class doesn't guarantee order so we have
+ // to write the file using a Writer.
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ properties.store(baos, null);
+ // The encoding can be either UTF-8 or ISO-8859-1, as any non ascii
character
+ // is transformed into a \\uxxxx sequence anyway
Review Comment:
The comment states "The encoding can be either UTF-8 or ISO-8859-1" but the
code only uses ISO-8859-1 encoding. This comment is misleading as the code
doesn't provide any UTF-8 path or option. Either the comment should be
corrected to reflect that only ISO-8859-1 is used, or the implementation should
support both encodings if that was the intent.
```suggestion
// The properties file is written using ISO-8859-1; any non-ASCII
character
// is transformed into a \\uxxxx sequence by Properties.store(...)
```
##########
src/main/java/org/apache/maven/archiver/PomPropertiesUtil.java:
##########
@@ -153,7 +133,7 @@ public void createPomProperties(
p.setProperty("version", version);
- createPropertiesFile(p, pomPropertiesFile, forceCreation);
+ createPropertiesFile(p, pomPropertiesFile.toPath());
Review Comment:
The forceCreation parameter is now ignored in the new implementation. The
old code used this parameter to check if the file already has the same content
(via sameContents method) and would skip creation if forceCreation was false
and content was identical. This behavioral change means the file will now
always be written even if its content hasn't changed, potentially affecting
build performance and incremental build behavior.
##########
src/main/java/org/apache/maven/archiver/PomPropertiesUtil.java:
##########
@@ -51,44 +51,25 @@ private Properties loadPropertiesFile(File file) throws
IOException {
}
}
- private boolean sameContents(Properties props, File file) throws
IOException {
- if (!file.isFile()) {
- return false;
+ private void createPropertiesFile(Properties properties, Path outputFile)
throws IOException {
+ Path outputDir = outputFile.getParent();
+ if (outputDir != null) {
+ Files.createDirectories(outputDir);
}
-
- Properties fileProps = loadPropertiesFile(file);
- return fileProps.equals(props);
- }
-
- private void createPropertiesFile(Properties properties, File outputFile,
boolean forceCreation)
- throws IOException {
- File outputDir = outputFile.getParentFile();
- if (outputDir != null && !outputDir.isDirectory() &&
!outputDir.mkdirs()) {
- throw new IOException("Failed to create directory: " + outputDir);
- }
- if (!forceCreation && sameContents(properties, outputFile)) {
- return;
- }
-
- try (PrintWriter pw = new PrintWriter(outputFile, "ISO-8859-1");
- StringWriter sw = new StringWriter()) {
-
- properties.store(sw, null);
-
- List<String> lines = new ArrayList<>();
- try (BufferedReader r = new BufferedReader(new
StringReader(sw.toString()))) {
- String line;
- while ((line = r.readLine()) != null) {
- if (!line.startsWith("#")) {
- lines.add(line);
- }
- }
- }
-
- Collections.sort(lines);
- for (String l : lines) {
- pw.println(l);
- }
+ // For reproducible builds, sort the properties and drop comments.
+ // The java.util.Properties class doesn't guarantee order so we have
+ // to write the file using a Writer.
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ properties.store(baos, null);
+ // The encoding can be either UTF-8 or ISO-8859-1, as any non ascii
character
+ // is transformed into a \\uxxxx sequence anyway
+ String output = Arrays.stream(
+
baos.toString(StandardCharsets.ISO_8859_1.name()).split("\\r?\\n"))
+ .filter(line -> !line.startsWith("#"))
+ .sorted()
+ .collect(Collectors.joining("\n", "", "\n"));
Review Comment:
The new sorting behavior introduced for reproducible builds lacks test
coverage. While existing tests verify that properties are present with correct
values, they don't verify that the properties are sorted in the output file.
Consider adding a test that reads the pom.properties file as text and verifies
that the property lines are in sorted order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
