Bug 2189633
Summary: | Review Request: mkosi-initrd - Generator for initrd images using distro packages | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Zbigniew Jędrzejewski-Szmek <zbyszek> | ||||||
Component: | Package Review | Assignee: | Neal Gompa <ngompa13> | ||||||
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | ngompa13, package-review | ||||||
Target Milestone: | --- | Flags: | ngompa13:
fedora-review+
|
||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
URL: | https://github.com/systemd/mkosi-initrd | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | If docs needed, set a value | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2023-05-17 01:31:15 UTC | Type: | --- | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Bug Depends On: | |||||||||
Bug Blocks: | 2203221 | ||||||||
Attachments: |
|
Description
Zbigniew Jędrzejewski-Szmek
2023-04-25 18:24:25 UTC
Taking this for review. Initial spec review: > %global forgeurl https://github.com/systemd/mkosi-initrd/ > %global distprefix %nil > %forgemeta Please don't use the forge macros, as they are unmaintained and only exist because Go and Font macros require them: https://pagure.io/packaging-committee/pull-request/1270 > %global pkgroot /usr/lib/mkosi-initrd Three things: 1. Why is this in the middle of the spec? 2. Why isn't this "%{_prefix}/lib"? 3. Why is this in /usr/lib and not /usr/libexec? > /usr/lib/kernel/install.d/50-mkosi-initrd.install Please use "%{_prefix}/lib" > %files > %doc README.md > %doc docs/fedora.md License file is missing. (In reply to Neal Gompa from comment #2) > Please don't use the forge macros Done. > > %global pkgroot /usr/lib/mkosi-initrd > 1. Why is this in the middle of the spec? It's defined right above the first use. It's just a helper variable to avoid repeating the same thing a few times. > 2. Why isn't this "%{_prefix}/lib"? > > > /usr/lib/kernel/install.d/50-mkosi-initrd.install > > Please use "%{_prefix}/lib" I changed it to use %{_prefix}. (Though I'm not convinced that this change makes sense. kernel-install doesn't support relocation. Various paths for plugins cannot be changed easily, they are documented and fixed in the code. So if in fact anyone tried to use a different %_prefix, this package wouldn't work at all. I know that using %_prefix is the received wisdom, but really, for many cases it doesn't do anything except obfuscate.) > 3. Why is this in /usr/lib and not /usr/libexec? The package is mostly some config files, so /usr/lib/<package> seems appropriate. This also follows the general systemd style. > License file is missing. Oops. I added it now upstream. Spec URL: https://in.waw.pl/~zbyszek/fedora/mkosi-initrd.spec SRPM URL: https://in.waw.pl/~zbyszek/fedora/mkosi-initrd-0.20230506g6ba68f1-1.fc39.src.rpm You still have the forge macros:
> %global forgeurl https://github.com/systemd/mkosi-initrd/
> %global distprefix %nil
> %forgemeta
Updated again. > Requires: systemd Do we actually require systemd on the host itself to produce the images? > Initrds created in this way support some common machine types, but no more: > - plain partitions > - LVM2 > - LUKS What about Btrfs? (In reply to Neal Gompa from comment #6) > > Requires: systemd > > Do we actually require systemd on the host itself to produce the images? We don't. Though this is a bit theoretical atm, because kernel-modules-core requires kernel-core which pulls in dracut and systemd. Those dependencies don't make sense and hopefully we can drop them one day. I dropped Required: systemd, kmod now. > > Initrds created in this way support some common machine types, but no more: > > - plain partitions > What about Btrfs? Btrfs and generally any other fs type supported by the kernel should work, as long as the underlying block storage is supported. Created attachment 1963345 [details]
The .spec file difference from Copr build 5849328 to 5897235
Copr build: https://copr.fedorainfracloud.org/coprs/build/5897235 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2189633-mkosi-initrd/fedora-rawhide-x86_64/05897235-mkosi-initrd/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string. > Version: 0.20230506g%{shortcommit} This is not valid for snapshot version: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots Suggestion: "0~%{commitdate}g%{shortcommit}" Fixed. Spec URL: https://in.waw.pl/~zbyszek/fedora/mkosi-initrd.spec SRPM URL: https://in.waw.pl/~zbyszek/fedora/mkosi-initrd-0~20230506g6ba68f1-1.fc39.src.rpm Created attachment 1963400 [details]
The .spec file difference from Copr build 5897235 to 5897335
Copr build: https://copr.fedorainfracloud.org/coprs/build/5897335 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2189633-mkosi-initrd/fedora-rawhide-x86_64/05897335-mkosi-initrd/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string. Package review notes: * Package is named appropriately * Package builds and installs * Package licensing is correctly identified and license files are installed * No serious issues from rpmlint Nit: You may want to consider "Supplements: mkosi" since it no longer drags in a bunch of stuff. PACKAGE APPROVED. The Pagure repository was created at https://src.fedoraproject.org/rpms/mkosi-initrd Thanks!
> You may want to consider "Supplements: mkosi" since it no longer drags in a bunch of stuff.
I'm not sure if I want to do this yet. The package has kernel-install plugin, which
shouldn't do anything without configuration, but bugs do happen. This is going to
remain very niche at least for a while, so I think it's fine if users have to install
explicitly.
FEDORA-2023-244b6a473e has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-244b6a473e FEDORA-2023-244b6a473e has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-244b6a473e \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-244b6a473e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates. FEDORA-2023-244b6a473e has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report. |