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

rimage: fix build error #144

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Conversation

aiChaoSONG
Copy link

Recent changes introduce build error, fix the build error.

Recent changes introduce build error, fix the build
error.

Signed-off-by: Chao Song <chao.song@linux.intel.com>
@@ -338,8 +338,6 @@ static int rimage_verify(EVP_PKEY *privkey, enum manver ver,struct hash_context
fprintf(stderr, "error: verify %s\n", err_buf);
}

break;

out:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you remove the break? It doesn't seem to belong to this patch if you really want to remove it.

Copy link

Choose a reason for hiding this comment

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

the break is in the middle of a function outside switch statement... so probably some copy/paste error, probably better just to remove. I mean this break was introduced in the same patch @aiChaoSONG is trying to fix here.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so we are fixing a copy/paste error in the previous patch .

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@marc-hb IIRC did we had a GH action to build rimage ?

Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @aiChaoSONG . Would be nice to add a reference to the commit that added the regression (d258ea2). Obviously some basic build tests would be needed here. Now that we are using west for rimage, we'll find issues like this much later. And e.g. today, we'd have a critical update for rimage, but we can update rimage as the current head doesn't build. :(

@@ -338,8 +338,6 @@ static int rimage_verify(EVP_PKEY *privkey, enum manver ver,struct hash_context
fprintf(stderr, "error: verify %s\n", err_buf);
}

break;

out:
Copy link
Member

Choose a reason for hiding this comment

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

ok, so we are fixing a copy/paste error in the previous patch .

@lgirdwood
Copy link
Member

Ok, let initially fix the build to unblock everyone and the testing infra. Second step is to see what's needed to get CI building rimage.

@lgirdwood lgirdwood merged commit 5bc2010 into thesofproject:main Mar 6, 2023
@marc-hb
Copy link
Contributor

marc-hb commented Mar 6, 2023

Second step is to see what's needed to get CI building rimage.

@marc-hb
Copy link
Contributor

marc-hb commented Mar 6, 2023

Actually, it's pretty easy to submit a temporary/draft/throw-away sof/west.yml test PR and make SOF CI run the FULL SOF test suites on test rimage changes BEFORE they're merged.

So maybe have more rimage CI is a bad idea after all, because it will only test rimage compilation and make people feel like it's enough.

@softwarecki
Copy link
Contributor

I think that testing through sof CI will not solve the problem either. We have sections of code that depend on openssl version. The error appeared into the section for a different version of openssl than is used in rimage CI I think that sof CI also uses only one openssl version. Rather, we should expand CI to use several openssl versions and perform image signatures and verification for different platforms.

@marc-hb
Copy link
Contributor

marc-hb commented Mar 6, 2023

In general I agree you can never have too much validation and too much test coverage - except for limited time and resources.

In this case though we should enforce a single openssl version per product. Maybe even a single openssl version across all SOF.

Why? Simply because we have very limited validation time and resources.

@aiChaoSONG did you find this failure in an unsupported openssl version?

@KokoSoft
Copy link

KokoSoft commented Mar 6, 2023

It looks like @aiChaoSONG is using openssl version above 3.0. If rimage pass CI in this PR thesofproject/sof#7196 , it means that it using older openssl same as rimage CI.

@lgirdwood
Copy link
Member

We need to deprecate support for older ssl versions. Version 3.0 has been out for some time now so should be easily available on all developer OS configurations.

@marc-hb
Copy link
Contributor

marc-hb commented Mar 6, 2023

tl;dr: openssl3 reached popular distros about 1 year ago but there is no stable Debian release with openssl 3 yet :-(
https://packages.debian.org/search?searchon=names&keywords=openssl

Ubuntu 22.04 was the first Ubuntu release with openssl3
https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=openssl&searchon=names
https://manpages.ubuntu.com/manpages/impish/man1/openssl.1ssl.html

Fedora apparently upgraded to openssl 3 in Fedora 36 which was released in May 2022
https://bodhi.fedoraproject.org/updates/?packages=openssl&page=2
Fedora 35 is not maintained any more.

@kv2019i
Copy link
Contributor

kv2019i commented Mar 7, 2023

@softwarecki @marc-hb And tackling different openssl versions is a bit of separate issue. Our PR CI will only use one version at time and a minimum bar is that any submitted code works with that version. If it doesn't, the code will break the CI for all new submitted code.

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.

8 participants