Fix GRUB fallback-from-removable logic introduced in c095eb56d8 #3950
Conversation
Commit c095eb5 was supposed to introduce logic such that if the `grub-install` command failed with a `--removable` flag, then another attempt would be made with such flag removed. This was broken because the `--removable` flag was kept in both cases (likely a copy-paste mistake). This has been an issue since, in all future iterations of the code. What this commit does is fix this logic, but also invert the cases tested: first test without `--removable`, then add it should that case fail, as this is the most sensible thing to do.
mintsuki
force-pushed
the
fix-grub-removable-fallback
branch
from
003d525 to
b5fc766
Compare
|
This seems like a hack and a "lets try until it works" thing. I'd rather this to be an actual logic rather than abusive errors as if-else cases |
|
Well, I wouldn't necessarily call this a hack per se, and in any case the original logic was broken regardless. I would also be okay with removing this fallback altogether and having users manually select the removable option from the menu (the one I added in a previous PR). |
|
Because --removable is always more reliable in both cases at least from personal experience |
Yes, but that is not a good argument for unconditionally using the The UEFI specification intends the removable path (on x86-64: For non-removable media, the intended way is to let the firmware know what bootloaders/EFI applications are available with NVRAM entries (as managed by Now, it is true that on some motherboards The way Debian's installer sorts this out is to simply do both, but let the user choose if they also want to use the removable location, with a short explanation of why they may want to do that. My opinion is this: the code as-is is broken, and it never made sense. We should either remove that fallback path entirely and only decide based on whether the user says they want to use the removable path or not, or we should do as this PR does currently, where the non-removable code path is tried first. |
|
From the Grub Wiki Page:
It is a better default I think, generally speaking. |
|
It is not a good default, it's a fallback. At the very least the user should be prompted about it like Debian does. Recently merged PR #3932 adds a screen that prompts the user whether they want to install to the removable location or not. |
|
For the sake of fixing this for now I'll merge the PR. The thing that I don't particularly like about it is that if a user does not set the removable setting now archinstall will simply go ahead and add it anyways on the failure. |
| except SysCallError: | ||
| pass | ||
|
|
||
| raise DiskError(f'Could not install GRUB to {self.target}{efi_partition.mountpoint}: {err}') from err |
There was a problem hiding this comment.
It looks like the fallback branch with --removable will always fall through and raise an exception here. Am I overlooking something?
As mentioned by @svartkanin on archlinux#3950, given we now have a way for the user to explicitly specify if they want to install to the removable location, having a fallback like this seems undesirable. On top of that, as mentioned by @correctmost on the same PR, the code that said PR introduced was bugged and would always raise an exception anyways.
As mentioned by @svartkanin on #3950, given we now have a way for the user to explicitly specify if they want to install to the removable location, having a fallback like this seems undesirable. On top of that, as mentioned by @correctmost on the same PR, the code that said PR introduced was bugged and would always raise an exception anyways.
4 participants
Commit c095eb5 was supposed to introduce logic
such that if the
grub-installcommand failed with a--removableflag, thenanother attempt would be made with such flag removed.
This was broken because the
--removableflag was kept in both cases (likely acopy-paste mistake). This has been an issue since, in all future iterations of
the code.
What this commit does is fix this logic, but also invert the cases tested:
first test without
--removable, then add it should that case fail, as this isthe most sensible thing to do.