GH-31: Allow setting payload coding and flags in configuration#104
GH-31: Allow setting payload coding and flags in configuration#104dwalluck wants to merge 1 commit intoctron:masterfrom
Conversation
| * If no payload coding is specified, then the default RPM compression method is used. | ||
| */ | ||
| @Parameter(property = "rpm.payloadCoding") | ||
| String payloadCoding; |
There was a problem hiding this comment.
Doesn't this also need the markdown documents changed?
There was a problem hiding this comment.
If payloadCoding is PayloadCoding.ZSTD by default why not define it here which might make the code slightly simpler? Also the javadoc says the default RPM compression method is used but that isn't quite right is it?
There was a problem hiding this comment.
So the default for RPM is actually "gzip 9". In Fedora, and RPM 6, they have been using "zstd 19". So, I am not sure what the default for RPM Builder should be.
You can check a particular RPM with
rpm -q --queryformat '%{PAYLOADCOMPRESSOR} %{PAYLOADFLAGS}\n' foo.rpm
| * then the default compression level for the {@link PayloadCoding payload coding} is used. | ||
| */ | ||
| @Parameter(property = "rpm.payloadFlags") | ||
| String payloadFlags; |
There was a problem hiding this comment.
Is this a bit complex to expose to consumers? Especially without knowing what valid strings are (i.e. "0", "1", "2") ?
There was a problem hiding this comment.
It is actually somewhat arbitrary. The docs explain it a bit. It's probably up to packager to validate it. I will make another pull request there.
There was a problem hiding this comment.
Validation of numeric value done in eclipse-packager/packager#74.
There was a problem hiding this comment.
On second thought, a real datatype would be better here instead of a string, and let packager do the conversion to/from the string?
There was a problem hiding this comment.
A real datatype does sound better but it still seems like a rather complex option to expose 🤷
@ctron Thoughts?
There was a problem hiding this comment.
So, it seems that RPM allows bogus payload flags to end up in the final RPM.
%define _binary_payload w91.gzdio
rpmbuild --ba ./fedora-release.spec
$ rpm -q --queryformat "%{PAYLOADCOMPRESSOR} %{PAYLOADFLAGS}\n" ../RPMS/noarch/fedora-release-41-31.noarch.rpm
gzip 91But, it obviously didn't use compression level 91. I think it used 9, if it's taking the first digit. We'd have to check the C code.
There was a problem hiding this comment.
Our issue is that:
- Different compressors take different options, though some are shared:
([0-9]|[0-9]+)for level for all, andT([0-9])+for threads for lzma, xz, and zstd - Payload flags are parsed differently based on the compressor. For example, for gzip/bzip2 only 0-9 is allowed for level, but for zstd, multiple digits are allowed.
| final Optional<PayloadCoding> optPayloadCoding = PayloadCoding.fromValue(this.payloadCoding); | ||
| final PayloadCoding payloadCoding = optPayloadCoding.orElseThrow(() -> new MojoExecutionException("Payload coding ''" + this.payloadCoding + "' is invalid")); | ||
| this.logger.info("Payload Coding: %s", payloadCoding.getValue()); | ||
| options.setPayloadCoding(payloadCoding); |
There was a problem hiding this comment.
While the default is GZIP should the default here be ZSTD given e.g. https://fedoraproject.org/wiki/Changes/Switch_RPMs_to_zstd_compression
There was a problem hiding this comment.
I think I based it on seeing this in RPM macros:
#%_source_payload w9.gzdio
#%_binary_payload w9.gzdioSo packager defaulted to gzip 9, not even gzip default. It's possible to change the default for rpm-builder, which is separate.
| The compressors `gzip`, `lzma`, `bzip2`,and `xz` support a range of compression levels between 0 (no compression) and 9. | ||
| Additionally, `gzip` supports the value -1 for the default compression level. | ||
|
|
||
| The compressor `zstd` supports a range of compression levels between 0 (default compression level) and 22. |
There was a problem hiding this comment.
Doesn't there need to be an addition to src/site/site.xml to add this to the menu?
dwalluck
force-pushed
the
feature/set-payload-options
branch
2 times, most recently
from
4219630 to
7e1f569
Compare
dwalluck
changed the title
dwalluck
force-pushed
the
feature/set-payload-options
branch
7 times, most recently
from
6e538fd to
33efb41
Compare
dwalluck
force-pushed
the
feature/set-payload-options
branch
from
33efb41 to
9498365
Compare
2 participants
Closes #31.