Feature verify flags #43

OliverMatz · 2019-09-13T11:45:56Z

Depends on the modifications eclipse/packager#6, otherwise there will be compile problems.

see ctron#42 Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

Requires modifications from eclipse/packager#6 Fix for ctron#41. Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

ctron

Only a few minor issues. But in general it looks good. Thank you!

I already changed the master branch to use 0.16.0, which is released now. So you might want to rebase.

ctron · 2019-09-16T12:31:27Z

src/main/java/de/dentrassi/rpm/builder/VerifyDetails.java

@@ -0,0 +1,159 @@
+package de.dentrassi.rpm.builder;


Please add a license header.

ctron · 2019-09-16T12:32:15Z

src/main/java/de/dentrassi/rpm/builder/VerifyDetails.java

+   private Boolean linkto;
+   private Boolean user;
+   private Boolean group;
+   private Boolean mtime;


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

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.

ctron · 2019-09-16T12:34:06Z

src/main/java/de/dentrassi/rpm/builder/VerifyDetails.java

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


This method doesn't seem to return anything.

oops. Fixed.

ctron · 2019-09-16T12:35:07Z

src/main/java/de/dentrassi/rpm/builder/VerifyDetails.java

+ * See https://github.com/ctron/rpm-builder/issues/41
+ */
+public final class VerifyDetails {
+   private Boolean md5;


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

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?

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

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.

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:

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

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.

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.

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

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

ctron · 2019-09-17T07:49:21Z

I just saw your change. Looks good to me. I only don't understand using Boolean vs boolean in the fields.

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

dadaistic javadoc to suppress warnings Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

OliverMatz added 3 commits September 13, 2019 13:23

New test EntryDetailsTest

4a4c8b1

see ctron#42 Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

EntryDetailsTest: verify return value of EntryDetails.apply(..)

cc92133

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

Allow to control verification flags.

4fe655e

Requires modifications from eclipse/packager#6 Fix for ctron#41. Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

OliverMatz mentioned this pull request Sep 13, 2019

Make configuration files being ignored during verify #41

Closed

OliverMatz added 2 commits September 16, 2019 13:12

upgrade org.eclipse.packager:packager-rpm to 0.16.0

6724b57

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

resolve merge conflict

30a22d1

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

ctron requested changes Sep 16, 2019

View reviewed changes

rename VerifyDetails.md5 to fileDigest, add javadoc

3bc534f

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

OliverMatz added 2 commits September 17, 2019 09:52

add license header

ab920be

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

primitive boolean rather than Boolean,

56c2459

dadaistic javadoc to suppress warnings Signed-off-by: Oliver Matz <oliver.matz@dataport.de>

ctron merged commit 6ce869e into ctron:master Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature verify flags #43

Feature verify flags #43

OliverMatz commented Sep 13, 2019

ctron left a comment

ctron Sep 16, 2019

ctron Sep 16, 2019

OliverMatz Sep 17, 2019

ctron Sep 16, 2019

OliverMatz Sep 17, 2019

ctron Sep 16, 2019

OliverMatz Sep 17, 2019

OliverMatz Sep 17, 2019

ctron Sep 17, 2019

OliverMatz Sep 17, 2019 •

edited

Loading

ctron Sep 17, 2019

OliverMatz Sep 17, 2019 •

edited

Loading

ctron commented Sep 17, 2019

Feature verify flags #43

Feature verify flags #43

Conversation

OliverMatz commented Sep 13, 2019

ctron left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverMatz Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverMatz Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

ctron commented Sep 17, 2019

OliverMatz Sep 17, 2019 •

edited

Loading

OliverMatz Sep 17, 2019 •

edited

Loading