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

UKI: document where UKI addons provided via package should be #91

Merged
merged 1 commit into from
May 28, 2024

Conversation

esposem
Copy link
Contributor

@esposem esposem commented Dec 11, 2023

Copy link
Member

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, somehow I missed the notification

specs/unified_kernel_image.md Outdated Show resolved Hide resolved
@esposem
Copy link
Contributor Author

esposem commented Mar 7, 2024

Pushed the new text, but still we don't have agreement regarding where to put the addons, so this PR is going on draft until that is not decided. Once decided, I just need to change the path in the text.

@esposem esposem changed the title UKI: document where UKI addons provided via package should be draft: UKI: document where UKI addons provided via package should be Mar 7, 2024
@esposem esposem changed the title draft: UKI: document where UKI addons provided via package should be Draft: UKI: document where UKI addons provided via package should be Mar 7, 2024
@esposem esposem marked this pull request as draft March 7, 2024 12:36
@esposem esposem changed the title Draft: UKI: document where UKI addons provided via package should be UKI: document where UKI addons provided via package should be May 22, 2024
@esposem esposem marked this pull request as ready for review May 22, 2024 13:19
@esposem
Copy link
Contributor Author

esposem commented May 23, 2024

So to sum up:
Global go to /usr/lib/modules/extra.d/ and local addons go in /usr/lib/modules/<uname-r>/<uki-name>.efi.extra.d/.

@bluca
Copy link
Member

bluca commented May 23, 2024

modules is a directory owned by kernel packages, adding an extra.d is too generic, the name needs to be more specific

@esposem
Copy link
Contributor Author

esposem commented May 23, 2024

modules is a directory owned by kernel packages, adding an extra.d is too generic, the name needs to be more specific

I'll go with uki.extra.d then

@behrmann
Copy link
Member

Looks good to me, but

I'll go with uki.extra.d then

seems to still be missing?

@esposem
Copy link
Contributor Author

esposem commented May 27, 2024

Apologies. I was sure I pushed :(

@poettering
Copy link
Collaborator

So the chosen dirs make sense to me, I am not yet in love with the text though. i.e. it starts out with covering EFI addons and sysexts and then uses the word "addons" for both, even though we (at least from a systemd pov) just use the word "addon" for the EFI PE wrapped thingies, and treat "sysext" and "confext" as something separated (since they are GPT wrapped thingies).

Hence, I think we should make clear initially that this spec covers all three, and then should continue to generically talk about them as "auxiliary resources" or some generic term like that.

but the spec should document the right suffix for each:

  • addon: *.addon.efi
  • sysext: *.sysext.raw
  • confext: *.confext.raw

Comment on lines 180 to 182
* for PE addons is `*.addon.efi`
* for sysext DDIs is `*.sysext.raw`
* for confext DDIs is `*.confext.raw`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* for PE addons is `*.addon.efi`
* for sysext DDIs is `*.sysext.raw`
* for confext DDIs is `*.confext.raw`
* for PE addons it is `*.addon.efi`
* for sysext DDIs it is `*.sysext.raw`
* for confext DDIs it is `*.confext.raw`

or maybe better

Suggested change
* for PE addons is `*.addon.efi`
* for sysext DDIs is `*.sysext.raw`
* for confext DDIs is `*.confext.raw`
* `*.addon.efi`, for PE addons,
* `*.sysext.raw`, for sysext DDIs, and
* `*.confext.raw` for confext DDIs.

While at it, you can also drop the double the second newline after this.

Copy link
Member

Choose a reason for hiding this comment

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

The paragraph is now missing any reference to the version format spec. While I agree that we need to find a wording, that cannot be misunderstood as this needing to be a versioned file name and can be something like machineid (though that's valid under the version format spec), we need to be clear, that the name must be valid under the spec, e.g. not use characters disallowed by it.

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 am afraid that I wouldn't know what to write here. I personally think that the sentence I had initially was good, just as in the UKI name, so if you have any proposal feel free to write it here.

specs/unified_kernel_image.md Outdated Show resolved Hide resolved
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
hector-cao pushed a commit to hector-cao/virt-firmware that referenced this pull request May 28, 2024
Looks like default is now /usr/lib/modules:
uapi-group/specifications#91

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
specs/unified_kernel_image.md Outdated Show resolved Hide resolved
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

We've bikeshedded this to a gleaming polish ;) Can't do no more. Let's merge.

@keszybz keszybz merged commit ff86a74 into uapi-group:main May 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants