OSInside / kiwi

KIWI - Appliance Builder Next Generation
https://osinside.github.io/kiwi
GNU General Public License v3.0
289 stars 146 forks source link

Add new eficsm type attribute #2590

Closed schaefi closed 1 month ago

schaefi commented 1 month ago

Allow to produce EFI/UEFI images without hybrid CSM capabilities. This Fixes #2407

Conan-Kudo commented 1 month ago

Why add new modes instead of another control flag?

schaefi commented 1 month ago

Why add new modes instead of another control flag?

I decided so because a new control attribute makes sense when it can be combined with a number of different settings. However, in this case a new attribute, let's name it csm="true|false" would only make sense in combination with firmware="efi|uefi" on x86 and would be meaningless for anything else. That's why I rather added the two new mode names

Conan-Kudo commented 1 month ago

We already have arch-specific attributes, though. efi* attributes and force_mbr are already specific to some arches. I don't think having efi_csm would be that much worse than what we have now. It can be a no-op on non-x86 arches, which is fine.

Conan-Kudo commented 1 month ago

And efi_only|uefi_only is going to be confusing for people and cause problems down the road when we have to rip it out once we can't support hybrid boot anymore.

schaefi commented 1 month ago

We already have arch-specific attributes, though. efi* attributes and force_mbr are already specific to some arches. I don't think having efi_csm would be that much worse than what we have now. It can be a no-op on non-x86 arches, which is fine.

I'm not referring to the arch specific bit, my point is that csm yes or no is only relevant for EFI. EFI is a firmware setting connected to a layout design and that's what the firmware specification indicates. Also see the updated documentation which should make it clear for every user.

schaefi commented 1 month ago

And efi_only|uefi_only is going to be confusing for people and cause problems down the road when we have to rip it out once we can't support hybrid boot anymore.

why is this confusing ? I can't follow why it should be problematic to rip it out ?

ripping out a supported value from an attribute is easy, a simple XSL stylesheet that changes the value to whatever we want ripping out an attribute is in the same way easy, a simple XSL stylesheet that drops the attribute

The complexity of moving forward regarding the schema is the same

So we need to get clarity on what's confusing about "efi_only" ?

schaefi commented 1 month ago

Given there is a csm attribute people could specify

firmware="ofw" csm="true"

That's confusing and not even possible. And adding a rule set in the schema for useful combinations is difficult and an effort I'm not willing to implement because we could just come up with a not confusing firmware name for EFI without CSM, for which I thought "efi_only" plus docs should do the trick

Conan-Kudo commented 1 month ago

But don't we already have that problem with the other attributes?

schaefi commented 1 month ago

But don't we already have that problem with the other attributes?

Which attribute combination do you have in mind ?

Conan-Kudo commented 1 month ago

efipartsize and force_mbr are both two attributes that make no sense with firmware="ofw", but are still a valid combination that can be set.

Conan-Kudo commented 1 month ago

Given there is a csm attribute people could specify

firmware="ofw" csm="true"

That's confusing and not even possible. And adding a rule set in the schema for useful combinations is difficult and an effort I'm not willing to implement because we could just come up with a not confusing firmware name for EFI without CSM, for which I thought "efi_only" plus docs should do the trick

If we call it efi_csm then I think it falls under the same bucket as the other efi* options we have as attributes that obviously don't make sense for non-EFI systems but exist alongside them anyway.

schaefi commented 1 month ago

yes it's true, there are more attributes that can be combined into non-sense combinations. Ok let's add another attribute but at least name it in a way that sort of tells the user it has something in common with EFI. I like eficsm=true|false

Conan-Kudo commented 1 month ago

Can we squash this down to two commits for eficsm + tests? We don't need the historical stuff in the final merge.

schaefi commented 1 month ago

Can we squash this down to two commits for eficsm + tests? We don't need the historical stuff in the final merge.

yes sure, I just need to complete my local build tests and once all is good I will squash into a good set. Just give me a few minutes, I'm an old man :)

schaefi commented 1 month ago

@Conan-Kudo ok done so far. All other tests will be done through the Staging builds