Jump to conversation
Unresolved conversations (9)
@ausil ausil Mar 22, 2023
sdubby does not exist
pyanaconda/modules/storage/bootloader/efi.py
keszybz ausil
poncovka
@keszybz keszybz Jan 28, 2023
"grub2" ?
docs/release-notes/systemd-boot.rst
@keszybz keszybz Jan 28, 2023
Strictly speaking, sd-boot has a 32-bit variant. I think it isn't tested much, and it's fine not to even try it from Anaconda, but it exists.
pyanaconda/modules/storage/bootloader/efi.py
@keszybz keszybz Jan 28, 2023
> `env_prune=['MALLOC_PERTURB_']` Why is this needed?
...nda/modules/storage/bootloader/systemd.py
jlinton
@keszybz keszybz Jan 28, 2023
`updateloaderentries` is from https://bugzilla.redhat.com/show_bug.cgi?id=2134972. As I wrote the ticket, please let's not do this at all. `updateloaderentries` is modifying some existing on-disk files. It would be nicer to just write the with the right contents from the start. But if we can't do that, then a bit of python should good enough: ```py def clean_up_loader_entry(filename, args): with open(filename, 'r+') as f: lines = [] for line in f: if line.startswith('options'): filtered = (opt for opt in line.split()[1:] if not opt.startswith('inst.') and not opt.startswith('BOOT_IMAGE')) lines += [' '.join((f'options {args}', *filtered)) + '\n'] else: lines += [line] f.seek(0) f.write(''.join(lines)) f.truncate() ```
...nda/modules/storage/bootloader/systemd.py
jlinton
@keszybz keszybz Jan 28, 2023
"for now we assume" — the installer is in full control, so there is no need hedge like this. Instead: "When the Linux installer is creating the partitions, it can create the ESP as large as needed."
...nda/modules/storage/bootloader/systemd.py
jlinton
@keszybz keszybz Jan 28, 2023
"its" Hmm, so this description doesn't sound right. "second boot menu": I guess you're thinking about the firmware menu, but generally it wouldn't be shown, so for normal use it's just "boot menu". Also, the kerel doesn't load the initrd, it happens earlier. Kernels with an efi stub also usually contain their own commandline, so injection of the parameters is an optional step… So this text would require some editing. But the bigger issue is that a description of systemd-boot features shouldn't be done here. Please instead refer to some docs (https://www.freedesktop.org/software/systemd/man/systemd-stub.html, https://www.freedesktop.org/software/systemd/man/systemd-boot.html ?). The feature set and details of operation of sd-stub/boot can change over time. I think a one-sentence description of systemd-boot would be enough.
...nda/modules/storage/bootloader/systemd.py
jlinton
@marcosfrm marcosfrm Oct 8, 2022
ditto
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
I see that the `ZIPL` class has an ` update_bls_args()` function that does something similar. Maybe there could be a common helper instead of having a separate script?
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
Resolved conversations (37)
@keszybz keszybz Jan 28, 2023
Is this supposed to be "SYSTMED" or "SDBOOT" ?
Outdated
...nda/modules/storage/bootloader/systemd.py
poncovka
@poncovka poncovka Jan 12, 2023
Could you move this text to the docstring of the `SystemdBoot` class?
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@poncovka poncovka Jan 12, 2023
Can you move the `storage` parameter at the beginning to make it consistent with other installation tasks, please?
Outdated
...odules/storage/bootloader/installation.py
@poncovka poncovka Jan 12, 2023
Please, add `SDBOOT` to the supported values in the docstring.
...nda/modules/storage/bootloader/factory.py
@poncovka poncovka Jan 12, 2023
Please, remove the extra space before `"efibootmgr"`.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@keszybz keszybz Dec 8, 2022
This would all look so much nicer with f-strings ;--]
...nda/modules/storage/bootloader/systemd.py
jkonecny12
@poncovka poncovka Nov 14, 2022
I would prefer to have `SystemdBoot` instead of `SYSTEMD` in the class names.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@poncovka poncovka Nov 14, 2022
Same here.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@poncovka poncovka Nov 14, 2022
I would encourage you to provide a more detailed message if you can and mark it for translations. You can do that with: `_("This string will be translated")`. This type of an error message is a bad practice and we should finally fix that.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@poncovka poncovka Nov 14, 2022
You can just ignore `args`.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
I would expect to get the UUID from the installer, but if it works, I am fine with that.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@poncovka poncovka Nov 14, 2022
You should be able to use `str(self.boot_args)`.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
Please, use `join_paths`.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
Please, use the `with` block again.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
Pleasse, remove logging from properties.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
Please, replace these with a single empty line.
Outdated
...nda/modules/storage/bootloader/systemd.py
@poncovka poncovka Nov 14, 2022
You should add `storage` to the task arguments and check `isinstance(self.storage.bootloader, SYSTEMD)`. The config value is a default. It doesn't have to reflect the actual state.
Outdated
...odules/storage/bootloader/installation.py
@poncovka poncovka Nov 14, 2022
Please, rename the class to `X64EFISystemdBoot`.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@poncovka poncovka Nov 14, 2022
Please, remove this property. It redefines the same name.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
jlinton
@poncovka poncovka Nov 14, 2022
Same here.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@poncovka poncovka Nov 14, 2022
Please, use `join_paths` from `pyanaconda.core.path` to create the path.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@poncovka poncovka Nov 14, 2022
This attribute can be removed since this configuration is unsupported.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@poncovka poncovka Nov 14, 2022
Please, change this section to: ``` with open("/sys/firmware/efi/fw_platform_size", "r") as f: value = f.readline().strip() ``` It will close the file at the end.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
poncovka
@poncovka poncovka Oct 20, 2022
Please, change the condition to: `if not self.stage2_required:`
Outdated
...aconda/modules/storage/bootloader/base.py
jlinton
@jlinton jlinton Oct 14, 2022
the stubby name here is going to have to change, I was only looking at the server rpms rather than everything, and there is already a stubby package. I think the plan is to open a package request as sdubby.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@marcosfrm marcosfrm Oct 8, 2022
Can be a non-Fedora distro... https://github.com/rhinstaller/anaconda/blob/anaconda-38.5-1/pyanaconda/modules/storage/bootloader/efi.py#L74-L79
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
should the name be systemd or sdboot? (here and in the following places).
Outdated
data/anaconda.conf
martinezjavier jlinton
jstodola
@martinezjavier martinezjavier Oct 7, 2022
ah, you are adding that future class here :)
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@martinezjavier martinezjavier Oct 7, 2022
Agreed, shim should only be needed for a future `x64EFISystemdBoot` class.
Outdated
pyanaconda/modules/storage/bootloader/efi.py
@martinezjavier martinezjavier Oct 7, 2022
that's strange, I wonder why shim would pull grub2...
Outdated
pyanaconda/modules/storage/bootloader/efi.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
Ah, I see that you are installing sd-boot using bootctl install. I thought that then there's no need to manually create a loader.conf. At least I didn't have to do it when migrated my machine from grub2 to use sd-boot...
...nda/modules/storage/bootloader/systemd.py
jlinton martinezjavier
@martinezjavier martinezjavier Oct 7, 2022
I wonder if all this should be done manually or could just `bootctl install` be invoked ?
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
I don't think you need this function either. Since that's only used by the `GRUB2` class.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
Do we even need the concept of stage2 for sd-boot? Since the kernel and initrd are in the ESP, so there isn't a need for a boot partition.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
Maybe instead of hardcoding this patch you could use `bootctl --print-esp-path` and make `_config_dir = "/loader/"` instead? That seems to be more aligned to what is expected, since AFAICT is mount point of the ESP + loader sub-dir.
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
why this is needed? I don't see being set for other bootloader classes like `ZIPL()`
Outdated
...nda/modules/storage/bootloader/systemd.py
jlinton
@martinezjavier martinezjavier Oct 7, 2022
it's odd indeed. There has been talks with the systemd folks to split it as a separate sub-package.
...nda/modules/storage/bootloader/systemd.py
keszybz jlinton
Conan-Kudo