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

Feature verify flags #43

Merged
merged 8 commits into from
Sep 18, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
<maven.version>3.3.9</maven.version>

<org.bouncycastle.version>1.56</org.bouncycastle.version>
<packager.version>0.16.0</packager.version>
</properties>

<prerequisites>
Expand Down Expand Up @@ -106,7 +107,7 @@
<dependency>
<groupId>org.eclipse.packager</groupId>
<artifactId>packager-rpm</artifactId>
<version>0.15.1</version>
<version>${packager.version}</version>
</dependency>

<dependency>
Expand Down
34 changes: 34 additions & 0 deletions src/main/java/de/dentrassi/rpm/builder/EntryDetails.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,35 @@ public class EntryDetails

private Boolean skip = false;

/**
* Controls verify flags.
* If null, the behaviour is unchanged, the verify flags bitmap will be set to -1, meaning: verify everthing.
* If (for example) empty, the verify flags bitmap will be set to 0, meaning: verify nothing.
* See https://github.com/ctron/rpm-builder/issues/41.
* <br>
* The following combination is a reasonable example in the sense of https://stackoverflow.com/a/38996621/11917731:
* <entry>
* ...
* <configuration>true</configuration>
* <noreplace>true</noreplace>
* <verify>
* <user>true</user>
* <group>true</group>
* </verify>
* </entry>
*/
private VerifyDetails verify;

public final VerifyDetails getVerify ()
{
return verify;
}

public final void setVerify ( final VerifyDetails verify )
{
this.verify = verify;
}

public void setMode ( final String mode )
{
this.mode = Short.parseShort ( mode, 8 );
Expand Down Expand Up @@ -210,6 +239,11 @@ public boolean apply ( final FileInformation info )
info.setMode ( this.mode );
didApply = true;
}
if ( this.verify != null )
{
this.verify.apply ( info );
didApply = true;
}
return didApply;
}

Expand Down
159 changes: 159 additions & 0 deletions src/main/java/de/dentrassi/rpm/builder/VerifyDetails.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
package de.dentrassi.rpm.builder;
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a license header.


import org.eclipse.packager.rpm.VerifyFlags;
import org.eclipse.packager.rpm.build.FileInformation;

import java.util.EnumSet;
import java.util.Set;

/**
* Each member of this class corresponds to one bit in the verify flags.
* See http://ftp.rpm.org/api/4.14.0/group__rpmvf.html.
* See https://github.com/ctron/rpm-builder/issues/41
*/
public final class VerifyDetails {
private Boolean md5;
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it be easier to use a boolean here, and default to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I copied the pattern from de.dentrassi.rpm.builder.EntryDetails. I assumed that there was some reason for that pattern?!
That pattern does have and issue though, see #42.
I am fine with changing the types in VerifyDetails it to primitive type boolean if you say so, but then what about EntryDetails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just changed to the type to boolean and fixed silly javadoc warnings.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm … interesting. The idea of EntryDetails was that it would override the current entry it processes. So if the value set, it will be set. If the value is "null" it will not change it.

Your original code is close to that, and now I understand what you had in mind.

I think it good to keep the Boolean, but change the "transfer" steps into a way, that they do process on the existing set. If I understand your code correctly, then this would provide the same behavior. If the entry is set, it will be applied (set or unset), if the value is "null", it won't change the flag.

Copy link
Contributor Author

@OliverMatz OliverMatz Sep 17, 2019

Choose a reason for hiding this comment

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

As far as the boolean flags in a bitmap are concerned, we do not have the option of "not changing" something. Either it is set or it is not. So the three values (null, true, false) will be mapped to two values (0,1). My cristisim for #42 is that for EntryDetails.readmy etc, that mapping is counterintuitive.

In this pull-request, there is one bitmap (org.eclipse.packager.rpm.build.RpmBuilder.FileEntry#verifyFlags).
In the old implementation, its value had the hard-coded value -1, so every flag was set.
My proposed change (or, rather, your proposal the way I understood it) is that the programmer has two options:

  1. He/she does not specify anything about verify-flags, then the behaviour is unchanged, all flags will be set and everthing will be verified; or
  2. He/she specifies a (possibly empty) list of those flags that shall be verified, then only those will be set in the bitmap and only the corresponding aspects will be verified.

Now it is not clear what exactly is meant by "if the value is 'null', it won't change the flag".

I am open to completely reconsider the question of how the programmer shall specify the verify-flags. It is a considerable idea to let the programmer specify a negative-list rather than a positive-list. This way, the default for each flag would be 'true' (or 1, respectively), and the user would specify the flags that shall be excluded from the bitmap.
However, while this might seem logical from the programmer's perspective, I consider it non-intuitive for the programmer.

BTW, I think that only very few combinations of these flags are relevant for practical purposes. If the user had to specify all flags to exclude if he/she wishes to exclude any, then this would IMHO severly complicate the usage.
My primary use-case (which is, in fact, the only one I can think of) is to customize the specification for configuration files. For such a file, the administrator of the target system may edit the file, which will change size, mtime, and digest at the same time.

So yet another approach to let the programmer specify the verify-flags would be to let him/her specify one of two or three bitmasks that actually make sense.

In conclusion: the current proposal (implemented with boolean rather than Boolean) is a reasonable approach, and for the sake of simplicitly I would favour to keep it that way.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I see your point … so, you consider the set of flags an entity by itself.

For the entry details, the idea was that every step would have the possibility to contribute to the final file entry. So you could have a rule, which matches e.g. /etc/ and flags all of those files as "configuration". But doesn't set any other attributes of the file.

Now I could imagine that someone might want to do a similar thing with the verification flags. Then again, no one asked for that 😀

So just let me know if your are fine with the current state, and I will merge it.

Copy link
Contributor Author

@OliverMatz OliverMatz Sep 17, 2019

Choose a reason for hiding this comment

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

I am satisfied with the current state and would appreciate your merging.

private Boolean size;
private Boolean linkto;
private Boolean user;
private Boolean group;
private Boolean mtime;
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer more "human readable" fields here. As they would be visible in the documentation and POM file I assume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These members and there corresponding bean properties correspond to the fields in the rpmVerifyFlags_e, see http://ftp.rpm.org/api/4.14.0/group__rpmvf.html. They also correspond to the rpm command line switches:
[--nolinkto] [--nofiledigest] [--nosize] [--nouser]
[--nogroup] [--nomtime] [--nomode] [--nordev]
[--nocaps]

I have just renamed the member md5 to fileDigest, as the corresponding command line switch --nofiledigest is now prefered against the obsolete --md5.
As far as the other names are concerned, I have no idea how to improve them.

private Boolean mode;
private Boolean rdev;
private Boolean caps;

public Boolean getMd5() {
return md5;
}

public void setMd5(Boolean md5) {
this.md5 = md5;
}

public Boolean getSize() {
return size;
}

public void setSize(Boolean size) {
this.size = size;
}

public Boolean getLinkto() {
return linkto;
}

public void setLinkto(Boolean linkto) {
this.linkto = linkto;
}

public Boolean getUser() {
return user;
}

public void setUser(Boolean user) {
this.user = user;
}

public Boolean getGroup() {
return group;
}

public void setGroup(Boolean group) {
this.group = group;
}

public Boolean getMtime() {
return mtime;
}

public void setMtime(Boolean mtime) {
this.mtime = mtime;
}

public Boolean getMode() {
return mode;
}

public void setMode(Boolean mode) {
this.mode = mode;
}

public Boolean getRdev() {
return rdev;
}

public void setRdev(Boolean rdev) {
this.rdev = rdev;
}

public Boolean getCaps() {
return caps;
}

public void setCaps(Boolean caps) {
this.caps = caps;
}

/**
* @see EntryDetails#apply(org.eclipse.packager.rpm.build.FileInformation)
* @return true if at least one flag has been set.
Copy link
Owner

Choose a reason for hiding this comment

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

This method doesn't seem to return anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. Fixed.

*/
void apply ( final FileInformation info )
{
final Set<VerifyFlags> verifyFlags = EnumSet.noneOf(VerifyFlags.class);
transfer(verifyFlags, this.md5, VerifyFlags.MD5 );
transfer(verifyFlags, this.size, VerifyFlags.SIZE );
transfer(verifyFlags, this.linkto, VerifyFlags.LINKTO );
transfer(verifyFlags, this.user, VerifyFlags.USER );
transfer(verifyFlags, this.group, VerifyFlags.GROUP );
transfer(verifyFlags, this.mtime, VerifyFlags.MTIME );
transfer(verifyFlags, this.mode, VerifyFlags.MODE );
transfer(verifyFlags, this.caps, VerifyFlags.CAPS );
info.setVerifyFlags ( verifyFlags );
}

private static void transfer(Set<VerifyFlags> target, Boolean val, VerifyFlags flag)
{
if (val == null)
{
return;
}
if (!val)
{
// target.remove(val); // not needed, target cannot contain flag
return;
}
target.add(flag);
}

@Override
public String toString() {
final StringBuilder ret = new StringBuilder("VerifyDetails{");
if (md5 != null) {
ret.append("md5,");
}
if (size != null) {
ret.append("size,");
}
if (linkto != null) {
ret.append("linkto,");
}
if (user != null) {
ret.append("user,");
}
if (group != null) {
ret.append("group,");
}
if (mode != null) {
ret.append("mode,");
}
if (rdev != null) {
ret.append("rdev,");
}
if (caps != null) {
ret.append("caps,");
}
ret.append("}");
return ret.toString();
}
}
61 changes: 61 additions & 0 deletions src/test/java/de/dentrassi/rpm/builder/EntryDetailsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package de.dentrassi.rpm.builder;

import org.eclipse.packager.rpm.FileFlags;
import org.eclipse.packager.rpm.build.FileInformation;
import org.junit.Test;

import java.util.Set;

import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;

/**
* Test class de.dentrassi.rpm.builder.EntryDetails.
*/
public class EntryDetailsTest
{
/**
* Verify that empty {@link EntryDetails} result in empty set of {@link FileFlags}.
*/
@Test
public void applyEmpty()
{
final EntryDetails entryDetails = new EntryDetails();
doTest(new FileFlags[] {}, false, entryDetails);
}

/**
* Verify that {@link EntryDetails#setReadme(java.lang.Boolean)} correctly controls {@link FileFlags#README}.
*/
@Test
public void applyReadmeTrue()
{
final EntryDetails entryDetails = new EntryDetails();
entryDetails.setReadme(true);
doTest(new FileFlags[] {FileFlags.README}, true, entryDetails);
}

/**
* False negative? See https://github.com/ctron/rpm-builder/issues/42
*/
@Test
public void applyReadmeFalse()
{
final EntryDetails entryDetails = new EntryDetails();
entryDetails.setReadme(false);
doTest(new FileFlags[] {FileFlags.README}, true, entryDetails); // questionable
}

/**
* invokes {@link EntryDetails#apply(org.eclipse.packager.rpm.build.FileInformation)}
* @param expectedResult expected return value of {@link FileInformation#getFileFlags()}
* @param expectedApplied expected return value of {@link de.dentrassi.rpm.builder.EntryDetails#apply(org.eclipse.packager.rpm.build.FileInformation)}
*/
private static void doTest(FileFlags[] expectedResult, boolean expectedApplied, final EntryDetails entryDetails)
{
final FileInformation fileInformation = new FileInformation();
assertEquals(expectedApplied, entryDetails.apply(fileInformation));
final Set<FileFlags> fileFlags = fileInformation.getFileFlags();
assertArrayEquals(expectedResult, fileFlags.toArray());
}
}
79 changes: 79 additions & 0 deletions src/test/java/de/dentrassi/rpm/builder/VerifyDetailsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package de.dentrassi.rpm.builder;

import org.eclipse.packager.rpm.FileFlags;
import org.eclipse.packager.rpm.VerifyFlags;
import org.eclipse.packager.rpm.build.FileInformation;
import org.junit.Test;

import java.util.Set;

import static junit.framework.TestCase.assertNull;
import static org.junit.Assert.assertArrayEquals;

/**
* See https://github.com/ctron/rpm-builder/issues/41
*/
public class VerifyDetailsTest
{
/**
* Verify that empty {@link VerifyDetails} result in empty set of {@link VerifyFlags}.
*/
@Test
public void applyEmpty()
{
final VerifyDetails verifyDetails = new VerifyDetails();
doTest(new VerifyFlags[] {}, verifyDetails);
}

@Test
public void applyUserOrGroupy()
{
final VerifyDetails verifyDetails = new VerifyDetails();
verifyDetails.setUser(true);
verifyDetails.setGroup(true);
doTest(new VerifyFlags[] {VerifyFlags.USER, VerifyFlags.GROUP}, verifyDetails);
}

/**
* Verify that {@link VerifyDetails#setLinkto(java.lang.Boolean)} with value true correctly controls {@link VerifyFlags#LINKTO}.
*/
@Test
public void applyLinktoTrue()
{
final VerifyDetails verifyDetails = new VerifyDetails();
verifyDetails.setLinkto(true);
doTest(new VerifyFlags[] {VerifyFlags.LINKTO}, verifyDetails);
}

/**
* Verify that {@link VerifyDetails#setLinkto(java.lang.Boolean)} with value false does not influence {@link VerifyFlags#LINKTO}.
* @see EntryDetailsTest#applyReadmeFalse()
*/
@Test
public void applyLinktoFalse()
{
final VerifyDetails verifyDetails = new VerifyDetails();
verifyDetails.setLinkto(false);
doTest(new VerifyFlags[] {}, verifyDetails);
}

/**
* Verify that not calling {@link VerifyDetails#apply(org.eclipse.packager.rpm.build.FileInformation)} results
* in a null VerifyDetails (as opposed to an empty set).
*/
@Test
public void noApply()
{
final FileInformation fileInformation = new FileInformation();
final Set<VerifyFlags> verifyFlags = fileInformation.getVerifyFlags();
assertNull(verifyFlags);
}

private static void doTest(VerifyFlags[] expectedResult, final VerifyDetails verifyDetails)
{
final FileInformation fileInformation = new FileInformation();
verifyDetails.apply(fileInformation);
final Set<VerifyFlags> verifyFlags = fileInformation.getVerifyFlags();
assertArrayEquals(expectedResult, verifyFlags.toArray());
}
}