Fixes grub splashImage documentation + implementation #40462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GRUB 2.0 supports png, jpeg and tga. This will use the image's suffix to load the right module. As jpeg module is named jpeg, jpg is renamed jpeg. If the user uses wrong image suffix for an image, it wouldn't work anyway. This will leave up to two additional left-over files in /boot/ if user switches through all the supported file formats. The module already left the png image if the user disabled the splash image.
samueldr
commented
May 14, 2018
| @@ -299,12 +299,16 @@ sub GrubFs { | |||
| copy $font, "$bootPath/converted-font.pf2" or die "cannot copy $font to $bootPath\n"; | |||
| } | |||
| if ($splashImage) { | |||
| # FIXME: GRUB 1.97 doesn't resize the background image if it | |||
There was a problem hiding this comment.
Before anyone asks, this comment has been "wrong" for a while.
GRUB 1.97 may still not resize the background image, but GRUB 2.02 does!
grahamc
reviewed
May 24, 2018
| $conf .= " | ||
| insmod png | ||
| if background_image " . $grubBoot->path . "/background.png; then | ||
| insmod " . substr($suffix, 1) . " |
There was a problem hiding this comment.
should we validate the suffix is one of N supported formats?
There was a problem hiding this comment.
What happens if the image format isn't supported?
There was a problem hiding this comment.
The previous behaviour:
- Image did not load, text mode console
(Just tested) the new behaviour:
- Exact same thing, image does not load, text mode console.
I'm guessing that an error message is quickly shown and hidden (error: file '/grub/x86_64-efi/ext.mod) as it would if I tried insmod ext in the console.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
This (first) fixes the documentation for the option, adding the details about the file format for grub 2.
Then, this fixes the implementation to make jpg and tga files loadable too. This will not work with a progressive jpeg, this is a known issue. This implementation will simply fail without background (grub behaviour) if a progressive jpeg image is used. I don't think (tell me if I'm wrong) that it's the module system's job to check for this.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)I can split the PR if it is wanted, though it will mean the documentation for the option will be slightly wrong until both commits are applied.