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

Add automatic lookup for UEFI_PFLASH_CODE/VARS to fix UEFI on Tumbleweed machines #1190

Merged
merged 1 commit into from Aug 6, 2019

Conversation

okurz
Copy link
Member

@okurz okurz commented Aug 2, 2019

This allows to simply set UEFI=1 and neither BIOS nor UEFI_PFLASH which will
lookup corresponding "code" and "vars" firmware files instead of failing.

The firmware files used by Fedora and Debian were already referencing "code"
files as single pflash targets so we do the same for the openSUSE package
which by now in openSUSE Factory does not supply the single code+vars image
anymore.

Currently os-autoinst-distri-opensuse sets UEFI_PFLASH which is still
supported from os-autoinst but will not work if the worker is a more recent
openSUSE Tumbleweed. Simply not specifying the variable will fix that problem
as soon as this commit is in.

Tested locally with a vars.json file in three variants, with UEFI_PFLASH
resolving the single code+vars file as well as without UEFI_PFLASH and no
explicit path to neither BIOS nor UEFI_FPLASH_CODE so that the firmware file
is automatically looked up. The third variant is to explicitly specify either
code or vars file which still works.

Related progress issue: https://progress.opensuse.org/issues/55052

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #1190 into master will increase coverage by 0.31%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   40.01%   40.33%   +0.31%     
==========================================
  Files          40       40              
  Lines        4868     4867       -1     
  Branches      828      827       -1     
==========================================
+ Hits         1948     1963      +15     
+ Misses       2588     2578      -10     
+ Partials      332      326       -6
Impacted Files Coverage Δ
bmwqemu.pm 53.84% <ø> (ø) ⬆️
backend/qemu.pm 35.68% <0%> (+3.03%) ⬆️
osutils.pm 76.92% <0%> (-1.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7147b64...6e42b19. Read the comment docs.

@okurz
Copy link
Member Author

okurz commented Aug 2, 2019

@richiejp ?

backend/qemu.pm Show resolved Hide resolved
bmwqemu.pm Show resolved Hide resolved
…eed machines

This allows to simply set UEFI=1 and neither BIOS nor UEFI_PFLASH which will
lookup corresponding "code" and "vars" firmware files instead of failing.

The firmware files used by Fedora and Debian were already referencing "code"
files as single pflash targets so we do the same for the openSUSE package
which by now in openSUSE Factory does not supply the single code+vars image
anymore.

Currently os-autoinst-distri-opensuse sets UEFI_PFLASH which is still
supported from os-autoinst but will not work if the worker is a more recent
openSUSE Tumbleweed. Simply not specifying the variable will fix that problem
as soon as this commit is in.

Tested locally with a vars.json file in three variants, with UEFI_PFLASH
resolving the single code+vars file as well as without UEFI_PFLASH and no
explicit path to neither BIOS nor UEFI_FPLASH_CODE so that the firmware file
is automatically looked up. The third variant is to explicitly specify either
code or vars file which still works.

Related progress issue: https://progress.opensuse.org/issues/55052
@richiejp
Copy link
Contributor

richiejp commented Aug 6, 2019

LGTM

okurz added a commit to okurz/os-autoinst-distri-opensuse that referenced this pull request Aug 6, 2019
okurz added a commit to okurz/os-autoinst-distri-opensuse that referenced this pull request Aug 6, 2019
@okurz
Copy link
Member Author

okurz commented Aug 6, 2019

I will look into tests in a separate PR because this would be something to discuss first.

@okurz okurz merged commit c597122 into os-autoinst:master Aug 6, 2019
@okurz okurz deleted the fix/uefi_for_new_ovmf branch August 6, 2019 13:10
@AdamWill
Copy link
Contributor

AdamWill commented Aug 6, 2019

ten points to anyone who can remember the entire sequence of how we got to where we are right now with all of this stuff...:P

@okurz
Copy link
Member Author

okurz commented Aug 7, 2019

@AdamWill uhhh, … do you mean I overlooked something? I tried to dig deep in the git logs to understand how we got there but I have not experienced the full history personally :)

@DrMullings
Copy link
Contributor

DrMullings commented Aug 7, 2019

Because of this change and the test suits not being adapted stuff is broken on osd
eg https://openqa.suse.de/tests/3221776
@foursixnine

@okurz
Copy link
Member Author

okurz commented Aug 7, 2019

@DrMullings the job you mentioned has hardcoded code+vars path but also UEFI_PFLASH. I did not know that this worked even in before but it shouldn't. Anyway, when you rebase your test code on latest master the variable UEFI_PFLASH should not be set anymore. Then this should work. I don't think further adjustements in test suites are necessary

@DrMullings
Copy link
Contributor

@okurz UEFI_PFLASH is being set in the test suite, we just checked that

@AdamWill
Copy link
Contributor

AdamWill commented Aug 7, 2019

@okurz I can't think of something specific, but we have changed this thing so damn often it's hard to be sure everything anyone does is gonna work (see @DrMullings issue for e.g.). "Just set UEFI and the code will figure out the file locations" is where we started, IIRC, waaaay back in time, then you could set BIOS to point to a single firmware file, then I think UEFI_PFLASH alone showed up and meant...something?...then UEFI_PFLASH_CODE and UEFI_PFLASH_VARS, and now this seems to bring back 'just set UEFI and we'll magically find stuff'. And unfortunately I don't think we put in enough tests to cover everything that worked.

I've already found weird gremlins in this stuff before, like at one point at least if you set a particular combination of variables the test would execute as a UEFI test, but the variable UEFI would not be truth-y, which had some odd consequences...

@okurz
Copy link
Member Author

okurz commented Aug 9, 2019

yes, true. This motivated me to at least start a test for "backend/qemu.pm" with #1191 . If people like it then I hope we can extend that one

coolgw pushed a commit to coolgw/os-autoinst-distri-opensuse that referenced this pull request Aug 19, 2019
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

5 participants