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

[SKIP SOF-TEST] .github: re-enable rimage workflows #8328

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Oct 16, 2023

Migrate rimage build and cppcheck workflows from
sof/tools/rimage/.github/workflows/ (where they're ignored) to sof/.github/workflows/

cc:

We already have these checks in sof.git so they don't need to be
migrated.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb force-pushed the github-rimage branch 7 times, most recently from bf56e6c to 0d75f46 Compare October 16, 2023 21:54
Migrate rimage build and cppcheck workflows from
sof/tools/rimage/.github/workflows/ (where they're ignored) to
sof/.github/workflows/

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Oct 16, 2023

Note I had to disable -Wpointer-arith because... it was never actually used in the standalone rimage repo. There's a ton of void * arithmetic in manifest.c

"If it's not tested then it does not work" - applies to every test, check and even CFLAGS.

# FIXME: add -Wpointer-arith
_CFLGS: -Werror -Wall -Wmissing-prototypes
-Wimplicit-fallthrough=3

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also have build target for windows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be great but I have no idea what it would take and I don't have any Windows system right now.

I can help with the Github Actions aspects if you want to try to add it later. For now this PR is merely trying to restore lost functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is recipe for windows:

export OPENSSL_PATH="c:/msys64/var/lib/pacman/local/openssl-1.1.1.n-1"
export CAT_PATH="c:/Program Files/Git/usr/bin"
export NINJA_PATH="c:/Program/ Files/ninja"
export MSYS_INSTALL_DIR="C:/msys64/usr/bin/"
export CMAKE_GENERATOR=Ninja

cmake -B zephyrproject/build-rimage -S zephyrproject/sof/rimage
cmake --build zephyrproject/build-rimage

Some of us are windows users and it would be nice if building would be checked for them...
Maybe open issue with that and leave it for future development?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of us are windows users and it would be nice if building would be checked for them...

Absolutely yes of course. But let's please first restore previous functionality and make sure rimage compiles without warning on ANYTHING :-)

export OPENSSL_PATH="c:/msys64/var/lib/pacman/local/openssl-1.1.1.n-1"
...

I doubt this will all work with Github's Windows image but we can probably re-use some of df1ba22

Maybe open issue with that and leave it for future development?

Depends on how long it takes, it could be quick (famous last words) and not really require a new issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I doubt this will all work with Github's Windows image but we can probably re-use some of df1ba22

Wait: this commit already compiles rimage on Windows (without -Werr -Wall). It's a bit lost in the long build logs but it's there. So unless you recommend a different toolchain (which one?) with a different set of warnings maybe then adding Windows here too wouldn't give anything new?

Copy link
Contributor

Choose a reason for hiding this comment

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

unless it will not change in the future then I don't see any problems.

@kv2019i kv2019i merged commit 8bbad9f into thesofproject:main Oct 18, 2023
40 checks passed
@marc-hb marc-hb deleted the github-rimage branch October 18, 2023 13:54
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.

5 participants