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

control rpm verification flags #6

Merged
merged 6 commits into from
Sep 16, 2019

Conversation

OliverMatz
Copy link
Contributor

see ctron/rpm-builder#41

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

see ctron/rpm-builder#41

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
rpm/pom.xml Outdated
<!-- show log output during tests using log4j as the slfj backend. -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know, this is just my disliking of log4j … could you use logback instead.

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 really do not care as long as I can somehow manage to see the log output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ctron
Copy link
Contributor

ctron commented Sep 11, 2019

This looks good! Thank you. Only a few things of cleaning up.

MODE( 1 << 6), // 'M'
RDEV( 1 << 7), // 'D'
CAPS( 1 << 8); // 'P'
// VERIFY_CONTEXTS = (1 << 15), VERIFY_FILES = (1 << 16), VERIFY_DEPS = (1 << 17),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for commenting the others out?

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 not understood the other flags. Besides I had the impression that there is some reason for that gap in the numbering. For my purposes, it really means no difference, so I am open to include those or leave it as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think is makes sense to add them all. In the end, this part is only passing them through. So not much processing is done here. For the Maven plugin, it may be a different story.

Copy link
Contributor Author

@OliverMatz OliverMatz Sep 13, 2019

Choose a reason for hiding this comment

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

O.K., I will comment in those ones and then push
... done

OliverMatz added a commit to Governikus/rpm-builder that referenced this pull request Sep 13, 2019
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>
Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
Copy link
Contributor

@ctron ctron left a comment

Choose a reason for hiding this comment

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

Sorry, I overlooked a few minor things.

@@ -0,0 +1,54 @@
package org.eclipse.packager.rpm;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry I overlooked this before, but this file is missing a license header.


// The purpose of the following constants is not clear to me.
// Do they refer to the same bitmask? Oliver Matz
// see diskussion in https://github.com/eclipse/packager/pull/6
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// see diskussion in https://github.com/eclipse/packager/pull/6
// see discussion in https://github.com/eclipse/packager/pull/6

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 + pushed.

* It should rather be "VerifyFlag" (singular), but I leave it this way, cf. {@link FileFlags}.
* The constants and their value are from http://ftp.rpm.org/api/4.14.0/group__rpmvf.html
* @since 0.15.2
* @author Oliver Matz
Copy link
Contributor

Choose a reason for hiding this comment

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

The project doesn't use @author tags. If you want your name added somehow, I can understand and we could achieve this using the NOTICE file instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed + pushed.

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
@ctron ctron merged commit 788d757 into eclipse:master Sep 16, 2019
ctron pushed a commit to ctron/rpm-builder that referenced this pull request Sep 18, 2019
* New test EntryDetailsTest
* see #42
* EntryDetailsTest: verify return value of EntryDetails.apply(..)
* Allow to control verification flags.
* Requires modifications from eclipse/packager#6
* Fix for #41.
* upgrade org.eclipse.packager:packager-rpm to 0.16.0
* resolve merge conflict
* rename VerifyDetails.md5 to fileDigest, add javadoc
* add license header
* primitive boolean rather than Boolean,
* dadaistic javadoc to suppress warnings

Signed-off-by: Oliver Matz <oliver.matz@dataport.de>
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.

None yet

2 participants