search

eclipse-archived / packagedrone

Eclipse Package Drone
http://eclipse.org/package-drone
Eclipse Public License 1.0
66 stars 39 forks source link

Add support for SHA256HEADER, FILE_DIGESTALGO, and Zstd compression #133

Closed dwalluck closed 5 years ago

dwalluck commented 6 years ago

Signed-off-by: David Walluck dwalluck@redhat.com

ctron commented 6 years ago

I just had the time to look into this PR. Wow! :smiley: I really like it.

There are a few minor things, but I definitely want to get this in.

I will need to file a few CQs for updated dependencies. This currently prevents the merging of this PR, I will take care of this, but it might take a while until they are processed by the Eclipse IP team.

I saw that the payload coding is a string, and the file digest is an integer. I would prefer them to be enums, if that is possible. As my understanding is, that they are both a limited set of choices, and putting in an unsupported value actually doesn't work as there would be no code to process this.

Maybe it would also make sense to extract the payload coding part as an actual interface, with different implementations such as Gzip, Xz, … only an idea, I would still merge it as it is. But that would make the payload codings actually pluggable.

I also add a few comments in the files themselves, only small things.

And thanks for this PR!

Btw, if your are interested, the "Eclipse Linux Package Tools" project proposal was made public last week 1. The idea is to focus this project on the RPM code of Package Drone and give a dedicated home. I definitely want to go ahead with this PR, before the project is approved. But I would encourage you to have a look.

dwalluck commented 6 years ago

I saw that the payload coding is a string, and the file digest is an integer. I would prefer them to be enums, if that is possible. As my understanding is, that they are both a limited set of choices, and putting in an unsupported value actually doesn't work as there would be no code to process this.

This is due to org.bouncycastle.bcpg.HashAlgorithmTags itself being an interface with int fields instead of an enum, unless you really want to wrap this class. Currently, putting an unsupported value will throw an exception, so it should not get through unnoticed.

I plan to create an enum which will reuse the org.bouncycastle.bcpg.HashAlgorithmTags values as these are the values used by RPM.

dwalluck commented 6 years ago

Maybe it would also make sense to extract the payload coding part as an actual interface, with different implementations such as Gzip, Xz, … only an idea, I would still merge it as it is. But that would make the payload codings actually pluggable.

You mean implementing factories similar to how commons-compress works? This seems like overkill to me right now. But, I will at least try moving all compression-related functionality to a separate class and see how you like it.

ctron commented 5 years ago

I am really, really sorry for the long delay in this PR. I merged it now and will take care of the small remaining changes myself.

Thanks for your patience on this!