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.

Sign up for GitHub

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

Jump to bottom

Add pe unittest #36188

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add pe unittest #36188

wants to merge 5 commits into from

Conversation

ani-sinha
Copy link
Contributor

Add pe unit tests

Took @anonymix007 's changes, rebased on top of latest main and make sure it builds and passes unit test in my workspace. See #35091

@ani-sinha ani-sinha mentioned this pull request Jan 27, 2025
Merged
@ani-sinha
Copy link
Contributor Author

ani-sinha commented Jan 27, 2025 •
edited
Loading

Failures are because it can't find dtc. Hopefully someone wiser knows where to add this package so that the CI is happy. I give up! ping @anonymix007 @poettering

@anonymix007
Copy link
Contributor

Looks like there is no agreement between distros on how to call the package for dtc, not sure what to do with this.
Ping @bluca @DaanDeMeyer @yuwata

@bluca
Copy link
Member

bluca commented Jan 27, 2025

If you check further into the mkosi config subdirectory, there are per-distribution package list, so add it there with the per-distro name instead of the general one

@anonymix007 anonymix007 force-pushed the add-pe-unittest branch 6 times, most recently from abc501f to d998600 Compare January 27, 2025 11:49
@anonymix007
Copy link
Contributor

I can't find where to put the dtc for the Fedora and noble CI jobs. Others jobs seem to be happy now

@anonymix007 anonymix007 force-pushed the add-pe-unittest branch 3 times, most recently from bdc418d to 18883fb Compare January 27, 2025 12:29
@bluca
Copy link
Member

bluca commented Jan 27, 2025

I can't find where to put the dtc for the Fedora and noble CI jobs. Others jobs seem to be happy now

Those are in separate repositories - but the build should not fail if it's not present, the test should be marked as SKIPPED instead

@DaanDeMeyer
Copy link
Contributor

Shouldn't we make this part of an integration test instead of a unit test? Seems a bit too dependency heavy to make it a unit test.

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed please-review PR is ready for (re-)review by a maintainer labels Feb 7, 2025
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Feb 7, 2025
@anonymix007 anonymix007 force-pushed the add-pe-unittest branch 2 times, most recently from 0cbdefc to 7d277f0 Compare February 7, 2025 15:47
yuwata
yuwata requested changes Feb 7, 2025
View reviewed changes
Copy link
Member

@yuwata yuwata left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks much better. But several more requests.

Could you swap the order of the last two commits? Without the new test, mkosi changes are meaningless.

tools/oss-fuzz.sh Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Feb 7, 2025
@anonymix007
Copy link
Contributor

Could you swap the order of the last two commits? Without the new test, mkosi changes are meaningless.

The intention was to never have a commit at which CI could be broken. But sure, will do

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Feb 7, 2025
@yuwata
Copy link
Member

yuwata commented Feb 7, 2025

Could you swap the order of the last two commits? Without the new test, mkosi changes are meaningless.

The intention was to never have a commit at which CI could be broken. But sure, will do

If mkosi config is not updated, the new test should be simply skipped on CI environments. Hence, the test should and can be introduced at first (though, that's not a big matter).

yuwata
yuwata approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

LGTM superficially.

I am not familiar with device-tree and so on.
Let's wait for other reviewers.

@yuwata yuwata removed the util-lib label Feb 7, 2025
yuwata
yuwata reviewed Feb 9, 2025
View reviewed changes
src/boot/log.h Outdated Show resolved Hide resolved
@anonymix007
boot: Introduce efi-userspace.c
8419647 
Used for dummy symbol definitions
@anonymix007
boot: Add devicetree.c to libefitest
05a8ace 
Fix some compilation issues while at it
@anonymix007
boot: Add pe.c and efi-firmware.c to libefitest
dfeb0aa 
Remove some functions unsuitable for userspace tests
Also add utility macros to obtain pointers to sections
@anonymix007
boot: Add PE section matching test
7a3d990
@anonymix007
mkosi: Add dtc
31c0312 
dtc is going to be used in unit tests
@ani-sinha
Copy link
Contributor Author

Ping! @poettering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluca bluca bluca left review comments

@yuwata yuwata yuwata approved these changes

@anonymix007 anonymix007 anonymix007 left review comments

@AKJUS AKJUS AKJUS approved these changes

@poettering poettering Awaiting requested review from poettering

@DaanDeMeyer DaanDeMeyer Awaiting requested review from DaanDeMeyer

Assignees
No one assigned
Labels
build-system meson mkosi please-review PR is ready for (re-)review by a maintainer sd-boot/sd-stub/bootctl tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ani-sinha @anonymix007 @bluca @DaanDeMeyer @yuwata @poettering @AKJUS