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

[blog] Drop usage of perl-Bootloader #280

Merged
merged 173 commits into from
Apr 12, 2016
Merged

[blog] Drop usage of perl-Bootloader #280

merged 173 commits into from
Apr 12, 2016

Conversation

jreidinger
Copy link
Member

Bootloader

There is plan to stop using perl-Bootloader as library for
yast2-Bootloader and now we realize it. Reasons for change were
different. The most visible user reason, is size of minimal system.
Grub2 for update of bootloader need just simple call of grub2-mkconfig,
but currently it uses whole perl-library with all its dependencies. It
also do whole hardware probing, which is later done by grub2-mkconfig
itself. So this change allow to simplify perl-Bootloader to be smaller
and make kernel update post script faster.
For developers main reason is to unify developed languages and also
reuse existing solution for file reading and parsing. For that reason
now yast2-bootloader uses rubygem cfa_grub2 which heavily depends on
augeas file parser/serializer. It allows to have code smaller and
simplifier. User visible change when using augeas is smarter editing
and keeping more comments. It is especially visible in installation,
when currently bootloader after installation of default grub2
configuration edit this file instead of overwritting with own version
like it was done in past.

Side-effect of this change is removal lot of work-arounds and simplify
installation work-flow for bootloader allowing easier future
improvement and open door for speed up a bit bootloader installation.
Also this change is taken as oppurtinity to improve code quality. To
ensure we do not break current work-flow we also improve test coverage
for bootloader module.

So some numbers showing effect of this change:
change size:

test coverage:

code climate rating:

@@ -110,6 +109,10 @@ def Propose
end
end

propose_os_probing
propose_terminal
propose_timout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a typo here (propose_timeout).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catch. I really indicate need of tests :)

@imobachgs
Copy link
Contributor

In general, looks good. Anyway, I miss some documentation to make this pre-review process easier :)

@jreidinger
Copy link
Member Author

travis failed due to not yet published config_files gem.

@ancorgs
Copy link
Contributor

ancorgs commented Nov 26, 2015

Just one note about a removed comment. Other than that, LGTM.


module Bootloader
# Represents non-EFI variant of GRUB2
class Grub2 < GRUB2Base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent. We should use only one of Grub, GRUB in class names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, currently it is WIP. I plan to rename GRUB2Base to Grub2Base

raise "cannot have secure boot without efi" if secure_boot && !efi
end

def execute(devices: nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param [Array,nil] devices identified by eg. "/dev/hda7", "/dev/ttyS0", "(hd8)", "/dev/by-zodiac/sagittarius" ?

@jreidinger jreidinger changed the title [do not merge] partial usage of config files gem Drop usage of perl-Bootloader Apr 12, 2016
@jreidinger
Copy link
Member Author

JFYI final code climate rating change 3.3 GPA(+1.57) https://codeclimate.com/github/yast/yast-bootloader/compare/config_files_proto

@coveralls
Copy link

Coverage Status

Coverage increased (+9.3%) to 78.276% when pulling 45b0a88 on config_files_proto into 4d776ca on master.

@@ -4,4 +4,7 @@ Yast::Tasks.configuration do |conf|
# lets ignore license check for now
conf.skip_license_check << /.*/
conf.install_locations["doc/autodocs"] = conf.install_doc_dir

conf.obs_project = "home:cwh:branches:YaST:Head"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTH: This is just a temporary OBS project isn't it? Then if would be nice to have a FIXME comment there...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good catch, it should be removed from master, thanks for noticing it

@coveralls
Copy link

Coverage Status

Coverage increased (+9.3%) to 78.276% when pulling 6a16ca8 on config_files_proto into 4d776ca on master.

@lslezak
Copy link
Member

lslezak commented Apr 12, 2016

LGTM (after a quick scan of the big and incomplete diff)

@jreidinger
Copy link
Member Author

thanks

@jreidinger jreidinger merged commit ce4c32d into master Apr 12, 2016
@jreidinger jreidinger changed the title Drop usage of perl-Bootloader [blog] Drop usage of perl-Bootloader Apr 13, 2016
@shundhammer shundhammer deleted the config_files_proto branch March 28, 2019 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants