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

Fix UEFI tools integration (should address issues in #1477, #1478) #1481

Merged
merged 2 commits into from Sep 12, 2017

Conversation

OliverO2
Copy link
Contributor

This change integrates UEFI tools on demand as determined by 320_include_uefi_env.sh.

It avoids replicating UEFI tests already done in 320_include_uefi_env.sh and just relies on a correctly set $USING_UEFI_BOOTLOADER variable. For such variable to be set correctly, 310_include_uefi_tools.sh is renamed to 330_include_uefi_tools.sh.

@OliverO2
Copy link
Contributor Author

Related: #1477, #1478

@OliverO2
Copy link
Contributor Author

Tested on Ubuntu 16.04.3 LTS with UEFI. BIOS-only tests outstanding.

Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

Any change that looks like this is a good one 😄
image

@@ -1,19 +1,10 @@
#
# 310_include_uefi_tools.sh
# 330_include_uefi_tools.sh
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to name the script in the script? If nobody sees a reason then let's remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a necessity. Inherited from 6e05b6f

@gozora
Copy link
Member

gozora commented Sep 11, 2017

Just for my understanding, this PR supersedes #1477, right ?

@OliverO2
Copy link
Contributor Author

Yes, it does.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

Wow!
How simple the code can become
when things are done straightforward.
Now even I start to understand what
there is acually meant to do ;-)

@jsmeix
Copy link
Member

jsmeix commented Sep 12, 2017

@gozora
could you merge it if it is also o.k. for you?

@jsmeix jsmeix added bug The code does not do what it is meant to do cleanup enhancement Adaptions and new features labels Sep 12, 2017
@jsmeix jsmeix added this to the ReaR v2.3 milestone Sep 12, 2017
@gozora gozora merged commit 1225ef9 into rear:master Sep 12, 2017
@gozora
Copy link
Member

gozora commented Sep 12, 2017

@jsmeix yes, I have no objections here.
@OliverO2 thanks for your contribution!

V.

@pcahyna
Copy link
Member

pcahyna commented Sep 12, 2017

I confirm that it fixes #1478 for me and looks cleaner than anything else.

@OliverO2
Copy link
Contributor Author

Thanks everyone!

@jsmeix
Copy link
Member

jsmeix commented Sep 12, 2017

@OliverO2
many thanks for your valuable contribution to ReaR
that did not only fix a particular issue but also cleaned up
overcomplicated code which could help a lot if there are
further issues in this area to get them also properly fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do cleanup enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants