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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Jun 8, 2023

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

/**
* @return Override umask.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is an override umask? The api doc should explain

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency with existing incomplete code is not a virtue.

Copy link

Choose a reason for hiding this comment

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

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.
 */

/**
* @return Override umask.
*/
Integer getOverrideUmask();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

@mabrarov mabrarov Jun 8, 2023

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Added default method implementation.

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

/**
* Override of umask.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

@mabrarov mabrarov Jun 8, 2023

Choose a reason for hiding this comment

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

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.

?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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
Copy link

mabrarov commented Jun 8, 2023

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.

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

/**
* @return Override umask.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency with existing incomplete code is not a virtue.

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

/**
* Override of umask.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/**
* @return Override umask.
*/
Integer getOverrideUmask();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Mask --> mask

Copy link

@mabrarov mabrarov Jul 5, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link

Choose a reason for hiding this comment

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

Fixed

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

mabrarov commented Jul 6, 2023

Hi @elharo,

Regarding that comment:

  1. Integration test for the new configuration option was added.
  2. 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.

<version>1.0</version>

<properties>
<project.build.outputTimestamp>2023-06-07T10:18:31Z</project.build.outputTimestamp>
Copy link

@mabrarov mabrarov Jul 6, 2023

Choose a reason for hiding this comment

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

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:

  1. 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).
  2. 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:

  1. 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.
  2. 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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

  1. It definitely should be fixed in Plexus Archiver (but it is much more complex task).
  2. 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.

Copy link
Contributor

@elharo elharo Jul 7, 2023

Choose a reason for hiding this comment

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

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

Copy link

@mabrarov mabrarov Jul 7, 2023

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

@mabrarov mabrarov Jul 7, 2023

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

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.

<version>1.0</version>

<properties>
<project.build.outputTimestamp>2023-06-07T10:18:31Z</project.build.outputTimestamp>
Copy link
Contributor

@elharo elharo Jul 7, 2023

Choose a reason for hiding this comment

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

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


/**
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

the assembly

Copy link

Choose a reason for hiding this comment

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

Fixed

/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Thanks. Fixed.

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

Choose a reason for hiding this comment

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

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)

Copy link

@mabrarov mabrarov Jul 7, 2023

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

Renamed overrideUmask into umask to avoid confusion.

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.

3 participants