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

Make 'grub-mkconfig' command name configurable #56

Merged
merged 2 commits into from
Aug 15, 2018

Conversation

N-Parsons
Copy link
Contributor

I noticed that grub-btrfs.service was using grub2-mkconfig which is not available on my system (Manjaro), so I've rewritten the service to allow the command name to be a configuration option. I've set grub-mkconfig as the default, since that's the command name that I've encountered most often in recent years, but I'm happy to change it if you would like.

I found the pair of commands used in grub-btrfs.service quite unclear, and on the first run (or whenever grub-btrfs.cfg doesn't exist), both commands would be run and the configuration would be regenerated twice. To resolve this, I've combined the commands into a single if... else... statement.

I've also updated 10-update_grub.conf to use the configurable grub directory name and to use the improved command from grub-btrfs.service.

As a side note, I'm not sure how useful it is to retain 10-update_grub.conf given that the new grub-btrfs.* services provide a more general solution. The former works only when the systemd services are used, while the latter works for systemd, cron, and manual snapshots.

@Antynea
Copy link
Owner

Antynea commented Aug 5, 2018

Hello,
Thanks for your contribution
I'll do some testing and get back to you soon.

After some tests, several problem encountered that requires correction:

1- When i start systemd service (systemctl start grub-btrfs.service),
if the file /boot/grub/grub-btrfs.cfg doesn't exist,
grub-mkconfig command is executed but if no snapshot exists,
the grub-submenu isn't created (in grub.cfg) and a /boot/grub/grub-btrfs.cfg file is created but "empty".

2- I create a snapshot, and I restart the service (systemctl restart grub-btrfs.service).
The script detects the presence of the file /boot/grub/grub-btrfs.cfg (previously created, but empty) and will not execute grub-mkconfig.
The file /boot/grub/grub-brfs.cfg will be populated, but the sub-menu will not appear in grub (because it was not written the first time, nor this time, since grub-mkconfig hasn't been invoked).

3- Don't use grub-mkconfig prevents the "check_uuid_required" function from working (implemented by #52). Variables aren't referenced in the script.
However, I can rewrite the function so that the script doesn't need grub-mkconfig.

Of course, I know that I have to use the grub-btrfs.service file in conjunction with grub-btrfs.path.

But it was to highlight the bad behavior that I detected.


As a side note, I'm not sure how useful it is to retain 10-update_grub.conf given that the new grub-btrfs.* services provide a more general solution. The former works only when the systemd services are used, while the latter works for systemd, cron, and manual snapshots.

We can always keep the file, free to the user to make his choice :)


I noticed that grub-btrfs.service was using grub2-mkconfig which is not available on my system (Manjaro)

Implemented by #53.
@Crafter6432 said on the fedora distribution, the command was named grub2-mkconfig.

I personally think, that it is necessary to put by default the command most often encountered.

- Use 'grub-mkconfig' as the default
- Note that some systems use 'grub2-mkconfig'
- Update 10-update_grub.conf to use the configurable grub directory name
- Refactor the systemd commands for clarity and to avoid duplicate regeneration on first run
@N-Parsons
Copy link
Contributor Author

Good catch! I've fixed point 2 by changing the test to check whether the file exists and is non-empty, which means that point 1 will no longer cause an issue.

For point 3, if the check_uuid_required function can easily be rewritten, then that would be the ideal solution, but otherwise I could remove the partial regeneration method (ie. only use grub-mkconfig). I quite like the idea of just regenerating the snapshot sub-menu, but if it's necessary to completely regenerate the menu in order to support more system configurations, then I'd be ok with that.

@Antynea
Copy link
Owner

Antynea commented Aug 11, 2018

Using the grub-mkconfig source code, I was able to make the script standalone.
No longer need grub-mkconfig to regenerate the snapshot sub-menu.

Of course, grub-mkconfig is necessary to add a entry in Grub-menu

Can you add this at your PR please ?

I think everything should be ok now, and should be able to merge

@Antynea Antynea merged commit 35703e7 into Antynea:master Aug 15, 2018
@Antynea Antynea mentioned this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants