[MASSEMBLY-791] overrideUmask option to ensure permissions in packaged archive match … #148

gnodet · 2023-06-08T03:39:33Z

…requested ones.

JIRA issue: https://issues.apache.org/jira/browse/MASSEMBLY-791

elharo · 2023-06-08T11:05:27Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

@@ -223,4 +223,9 @@ public interface AssemblerConfigurationSource {
     * @return Override group name.
     */
    String getOverrideGroupName();
+
+    /**
+     * @return Override umask.


What is an override umask? The api doc should explain

I failed to find the place were overrideGroupName option (which I followed for overrideUmask) is described. Could you please point the right one?

Consistency with existing incomplete code is not a virtue.

Changed to

/** * Mask which is applied to permissions of files/directories before they are put into assembly. * If {@code null} then the mask is not explicitly configured and remains implementation-specific. */

elharo · 2023-06-08T11:06:48Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

+    /**
+     * @return Override umask.
+     */
+    Integer getOverrideUmask();


Adding a method to an interface is an incompatible change in public API that can break existing code. In Java 8 this should have a default implementation to avoid that.

Sorry, but I failed to understand why we need to support backward compatibility for this interface. What about org.apache.maven.plugins.assembly.AssemblerConfigurationSource#getOverrideGroupName method (which I followed in this pull request)?

I was under impression that we need to provide backward compatibility for plugin configuration from the point of view of POM only - this is the reason null value is supported (and means that respective configuration options is not defined).

You never know who your dependents are. It's easy to add a return null here, and then you don't break anyone.

Added default method implementation.

elharo · 2023-06-08T11:08:05Z

src/main/java/org/apache/maven/plugins/assembly/mojos/AbstractAssemblyMojo.java

@@ -430,6 +430,12 @@ public abstract class AbstractAssemblyMojo extends AbstractMojo implements Assem
    @Parameter
    private String overrideGroupName;

+    /**
+     * Override of umask.


If this shows up in user docs, better description is needed. If it doesn't no description is needed.

I followed description of overrideGroupName option here. What about:

Suggested change

* Override of umask.

* Overrides mask which is applied to permissions of files/directories before they are put into assembly. If null then umask option of Plexus Archiver is not configured and default mask defined by Plexus Archiver (022) is used.

?

Now that I see the description, does it even matter that this overrides another value? It would be simpler to just say this sets the umask directly, and also specify the default value used if one is not specified here. The rest is implementation detail.

Changed to

/** * Mask which is applied to permissions of files/directories before they are put into assembly. * If {@code null} then the mask is not explicitly configured and remains implementation-specific. */

When this option is not configured (null), then the umask remains Plexus Archiver implementation detail (which I know is 022, but I cannot put it into documentation of overrideUmask configuration option of Maven Assembly Plugin).

IMHO, we can not put 022 as default value of overrideUmask configuration option of Maven Assembly Plugin, because it breaks backward compatibility (in case someone uses another version of Plexus Archiver - the one which has different default value of umask option - with Maven Assembly Plugin and migrates to the new version of Maven Assembly Plugin with this pull request integrated).

mabrarov · 2023-06-08T20:04:19Z

FYI, this pull request doesn't fixes original issue reported in MASSEMBLY-791, but fixes (with additional plugin configuration which is required to be added by the user of plugin (!)) a new issue introduced in 3.6.0 version and leading to the same (unexpected/invalid) behavior as described in MASSEMBLY-791. Refer to the comment in MASSEMBLY-989 for details. There is a big chance that with this pull request MASSEMBLY-791 can be closed as resolved.

Note that this pull request lacks test(s). One of the reasons behind that is that existing tests (located in the target branch of this pull request) fail when executed on Windows.

@gnodet and @elharo, I may need your help with tests, because it seems that I won't have time to work on this pull request till July 2023.

elharo · 2023-06-09T11:38:23Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

@@ -223,4 +223,9 @@ public interface AssemblerConfigurationSource {
     * @return Override group name.
     */
    String getOverrideGroupName();
+
+    /**
+     * @return Override umask.


Consistency with existing incomplete code is not a virtue.

elharo · 2023-06-09T11:40:39Z

src/main/java/org/apache/maven/plugins/assembly/mojos/AbstractAssemblyMojo.java

@@ -430,6 +430,12 @@ public abstract class AbstractAssemblyMojo extends AbstractMojo implements Assem
    @Parameter
    private String overrideGroupName;

+    /**
+     * Override of umask.


Now that I see the description, does it even matter that this overrides another value? It would be simpler to just say this sets the umask directly, and also specify the default value used if one is not specified here. The rest is implementation detail.

elharo · 2023-06-09T11:41:12Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

+    /**
+     * @return Override umask.
+     */
+    Integer getOverrideUmask();


You never know who your dependents are. It's easy to add a return null here, and then you don't break anyone.

elharo

Needs tests
What happens if this is set to an integer that is not a valid umask?

elharo · 2023-07-05T12:54:35Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

@@ -223,4 +223,12 @@ public interface AssemblerConfigurationSource {
     * @return Override group name.
     */
    String getOverrideGroupName();
+
+    /**
+     * @return Mask which is applied to permissions of files/directories before they are put into assembly.


nit: Mask --> mask

Sorry, do you mean changing of case (mask)? It looks like documentation for all over methods with @return Javadoc starts from upper case after @return statement. Is it OK to not follow existing formatting in this case?

Yes, change the case per Javadoc conventions as published by Oracle. Always follow the documented rules. Do not copy what some random developer did 15 years ago. "The description begins with a lowercase letter if it is a phrase (contains no verb), or an uppercase letter if it is a sentence. End the phrase with a period only if another phrase or sentence follows it."

mabrarov · 2023-07-06T19:54:08Z

Hi @elharo,

Regarding that comment:

Integration test for the new configuration option was added.
The case when invalid value is specified for overrideUmask plugin configuration option is described in Javadoc for org.apache.maven.plugins.assembly.mojos.AbstractAssemblyMojo#overrideUmask field. This Javadoc is used for generation of plugin documentation - plugin.xml - so I find it sufficient.

Thank you for your help with review of this pull request.

mabrarov · 2023-07-06T20:02:11Z

src/it/projects/bugs/massembly-791/pom.xml

+  <version>1.0</version>
+
+  <properties>
+    <project.build.outputTimestamp>2023-06-07T10:18:31Z</project.build.outputTimestamp>


Current implementation of Plexus Archiver (used by Maven Assembly Plugin) - org.codehaus.plexus.archiver.AbstractArchiver class - applies its default umask (which is 022) only when configureReproducibleBuild method is called. Current version of Maven Assembly Plugin invokes that method only when project.build.outputTimestamp maven property is specified. It means that 0 as value of overrideUmask configuration option changes behavior of Maven Assembly Plugin only when project.build.outputTimestamp maven property is specified. It's sad (complicated for the users of Maven Assembly Plugin) behavior, but it's an issue of Plexus Archiver, which should not apply its default umask to file/directory permissions for the case when both conditions are met:

fileMode/directoryMode are defined for fileSet (as far as I understand, both fileMode and directoryMode options of fileSet have default values - 0644 and 0755 respectively - so it means that this condition is always true).

Assembly format (like tar, zip and jar) supports configuration of file/directory permissions independent of OS.

This is the reason overrideUmask configuration option of Maven Assembly Plugin is introduced:

It helps to workaround bug in Plexus Archiver (the case my team has) - users of Maven Assembly Plugin can specify 0 as overrideUmask configuration option to explicitly override umask which is sometimes (see above) implicitly used by Plexus Archiver.

It provides additional flexibility for the users of Maven Assembly Plugin (but they need to realize that not every assembly format - like dir - supports setting same permissions as defined in assembly descriptor or source file/directory has in an OS independent way).

I'm losing the plot here, but it sounds like maybe this is something that should be fixed in plexus-archvier instead? If a fix there could avoid adding extra complexity and API here that would be preferable.

It definitely should be fixed in Plexus Archiver (but it is much more complex task).

We still can implement overrideUmask in Maven Assembly Plugin, because:

It can be used as a quick workaround for the Plexus Archiver bug.

IMHO, we still may need overrideUmask configuration option in Maven Assembly Plugin for flexibility, i.e. it's not just quick & dirty workaround.

Additional public API is too heavyweight a solution for a bug that can be fixed at the source.

Maven users use Assembly plugin and not Plexus Archiver, so for the users of Maven it is a bug of Assembly plugin introduced in 3.6.0 version (by migration to the new version of Plexus Archiver). We can provide a quick workaround. Changing Plexus Archiver is too complex. It can require Plexus Archiver API change and change of Assembly plugin. Unfortunately, I won’t be able to help with this kind of changes (I am even not the author of this pull request).

Assembly plugin already has override* configuration options, which are close to overrideUmask configuration option (which are the same sort workaround). IMHO, it’s a trade-off which Maven maintainers decide if they agree or not. I tried my best to provide all details for making informed decision :)

I do not agree with new public API as a quick workaround. If the bug was introduced by a new version of plexus archiver, then revert to the old version as a workaround until plexus archiver is fixed.

New public API needs to be justified on its own merits.

I'm not sure "override" means the same thing in both cases. If I'm following this, in the other cases the override replaces the existing GIDs, UIDs, etc. set on the files being added to the assembly. In this case it's not doing that. Rather it's changing the behavior of the plugin.

Maven Assembly Plugin 3.6.0 is not compatible with older version of Plexus Archiver (that was the first thing I tried to workaround this bug), so there is no way to "revert to old version" of Plexus Archiver when using Maven Assembly Plugin 3.6.0. Instead my team has to stay on 3.5.0 version of Maven Assembly Plugin (which cases some "false" warnings when used with Maven 3.9.x).

I'm not sure "override" means the same thing in both cases. If I'm following this, in the other cases the override replaces the existing GIDs, UIDs, etc. set on the files being added to the assembly. In this case it's not doing that. Rather it's changing the behavior of the plugin.

I meant the following Assembly plugin configuration options:

overrideUid

overrideUserName

overrideGid

overrideGroupName

are similar to this pull request, because instead of setting these values in Assembly plugin configuration, they had to be a part of respective sections of assembly descriptor - just like fileMode and directoryMode are part of fileSet section of assembly descriptor. Unfortunately, that change was more complex and required changes in API of Plexus Archiver (which I was working on). It seems that maintainers of Assembly plugin decided to chose simpler (but less generic / agile) path, which is the same as this pull request suggests.

elharo · 2023-07-07T11:35:57Z

src/it/projects/bugs/massembly-791/pom.xml

+  <version>1.0</version>
+
+  <properties>
+    <project.build.outputTimestamp>2023-06-07T10:18:31Z</project.build.outputTimestamp>


Additional public API is too heavyweight a solution for a bug that can be fixed at the source.

elharo · 2023-07-07T11:43:53Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

+
+    /**
+     * @return mask which is applied to permissions of files/directories before they are put into assembly.
+     * If {@code null} then the mask is not explicitly configured and remains implementation-specific.


Is it really implementation specific? Not just that there simply isn't a mask? How many implementations are there?

Yes, it is. It is implementation-specific, because it depends on Plexus Archiver internals. Refer to that comment for details, please.

elharo · 2023-07-07T11:44:06Z

src/main/java/org/apache/maven/plugins/assembly/AssemblerConfigurationSource.java

@@ -223,4 +223,12 @@ public interface AssemblerConfigurationSource {
     * @return Override group name.
     */
    String getOverrideGroupName();
+
+    /**
+     * @return mask which is applied to permissions of files/directories before they are put into assembly.


the assembly

elharo · 2023-07-07T11:45:44Z

src/main/java/org/apache/maven/plugins/assembly/mojos/AbstractAssemblyMojo.java

+    /**
+     * Override of mask which is applied to permissions of files/directories before they are put into assembly.
+     * If {@code null} then the mask is not explicitly configured and remains implementation-specific.
+     * If invalid value is specified - like negative value - then behaviour is implementation specific, i.e. depends


an invalid value
such as a negative value
the behavior
That is, it depends on the underlying
of assembly --> the assembly

Thanks. Fixed.

elharo · 2023-07-07T11:54:32Z

src/main/java/org/apache/maven/plugins/assembly/mojos/AbstractAssemblyMojo.java

+     * on underlying library which is used for building of assembly.
+     */
+    @Parameter
+    private Integer overrideUmask;


Does this actually override a umask or does it set a umask? In the other cases above — GID, username — these are set on the files and may vary from one file to the next. umask, however, is not a per-file property. It's applied to the permissions of the file (which is a per-file property)

It overrides umask at Assembly plugin level (i.e. for all files/directories) which otherwise is implementation specific (can be set by Plexus Archiver under some conditions or can be undefined), but is still plugin configuration scoped. Refer to that comment for details, please. override word means here overriding of what Plexus Archiver does under the hood and not overriding per file/directory attribute. I understand that set is more natural... do you find umask a better name than overrideUmask?

Renamed overrideUmask into umask to avoid confusion.

…e match expected ones.

elharo requested changes Jun 8, 2023

View reviewed changes

mabrarov force-pushed the feature/override_umask branch from 80343a9 to 257a6bc Compare June 8, 2023 19:52

elharo requested changes Jun 9, 2023

View reviewed changes

mabrarov force-pushed the feature/override_umask branch from 257a6bc to 583c538 Compare July 5, 2023 12:49

elharo requested changes Jul 5, 2023

View reviewed changes

mabrarov force-pushed the feature/override_umask branch 3 times, most recently from 37a1341 to eba6a6a Compare July 6, 2023 19:46

mabrarov reviewed Jul 6, 2023

View reviewed changes

elharo requested changes Jul 7, 2023

View reviewed changes

mabrarov force-pushed the feature/override_umask branch from eba6a6a to f9c2890 Compare July 7, 2023 13:57

[MASSEMBLY-791] umask option to ensure permissions in packaged archiv…

679ede9

…e match expected ones.

mabrarov force-pushed the feature/override_umask branch from f9c2890 to 679ede9 Compare July 8, 2023 09:24

	* Override of umask.
	* Overrides mask which is applied to permissions of files/directories before they are put into assembly. If null then umask option of Plexus Archiver is not configured and default mask defined by Plexus Archiver (022) is used.

[MASSEMBLY-791] overrideUmask option to ensure permissions in packaged archive match … #148

Are you sure you want to change the base?

[MASSEMBLY-791] overrideUmask option to ensure permissions in packaged archive match … #148

Conversation

gnodet commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov commented Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov commented Jul 6, 2023

mabrarov Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

mabrarov Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabrarov Jun 8, 2023 •

edited

Loading

mabrarov Jun 8, 2023 •

edited

Loading

mabrarov commented Jun 8, 2023 •

edited

Loading

mabrarov Jul 5, 2023 •

edited

Loading

mabrarov Jul 6, 2023 •

edited

Loading

elharo Jul 7, 2023 •

edited

Loading

mabrarov Jul 7, 2023 •

edited

Loading

mabrarov Jul 7, 2023 •

edited

Loading

elharo Jul 7, 2023 •

edited

Loading

mabrarov Jul 7, 2023 •

edited

Loading