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

efitools: fix do_deploy and keys check #1525

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

Conversation

quaresmajose
Copy link
Member

No description provided.

@quaresmajose quaresmajose requested a review from a team October 29, 2024 11:17
@quaresmajose
Copy link
Member Author

Also requires foundriesio/lmp-manifest#463

Copy link
Contributor

@ldts ldts left a comment

Choose a reason for hiding this comment

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

${D} can be changed and used as a temporary storage however the new data will not be packed.
This is not a problem in this case since this firmware is being picked from the deploy folder. However - after our conversation- I understand it is formally incorrect and confusing to the reader using ${D} that way.

@ldts
Copy link
Contributor

ldts commented Oct 29, 2024

@quaresmajose the commit message is wrong. UnLock.efi is created at compile time. It is however signed at install time

@quaresmajose
Copy link
Member Author

@quaresmajose the commit message is wrong. UnLock.efi is created at compile time. It is however signed at install time

right, I will change it to signed at install time

@quaresmajose
Copy link
Member Author

${D} can be changed and used as a temporary storage however the new data will not be packed. This is not a problem in this case since this firmware is being picked from the deploy folder. However - after our conversation- I understand it is formally incorrect and confusing to the reader using ${D} that way.

This is not true, ${D} is not a temporary storage and is the image folder of the recipe.
Only the do_install task should create or modify content on that folder.

${D} is the output folder of the do_install
${B} is the output folder of the do_compile
${DEPLOYDIR} is the output folder of the do_deploy

In a perfect world D/B/DEPLOYDIR should be read/write only for the mentioned tasks.
but that's not what happens and these directories can be modified at any time in any task,
just because this is possible doesn't mean it's correct.

there a lot of directories used by bitbake:

T = "${WORKDIR}/temp"
D = "${WORKDIR}/image"
S = "${WORKDIR}/${BP}"
B = "${S}"

more https://git.yoctoproject.org/poky/plain/meta/conf/bitbake.conf

@quaresmajose quaresmajose force-pushed the efitools branch 2 times, most recently from 708d9ae to 3c5125d Compare October 29, 2024 11:59
@ldts
Copy link
Contributor

ldts commented Oct 29, 2024

${D} can be changed and used as a temporary storage however the new data will not be packed. This is not a problem in this case since this firmware is being picked from the deploy folder. However - after our conversation- I understand it is formally incorrect and confusing to the reader using ${D} that way.

This is not true, ${D} is not a temporary storage and is the image folder of the recipe. Only the do_install task should create or modify content on that folder.

${D} is the output folder of the do_install ${B} is the output folder of the do_compile ${DEPLOYDIR} is the output folder of the do_deploy

In a perfect world D/B/DEPLOYDIR should be read/write only for the mentioned tasks. but that's not what happens and these directories can be modified at any time in any task, just because this is possible doesn't mean it's correct.

there a lot of directories used by bitbake:

T = "${WORKDIR}/temp"
D = "${WORKDIR}/image"
S = "${WORKDIR}/${BP}"
B = "${S}"

more https://git.yoctoproject.org/poky/plain/meta/conf/bitbake.conf

I did not say it is correct. I said that it can be used as temporary storage and the proof is that it does work as expected. What it is aboslutely not true is that it can not be used as temporary storage. Because it can.

A different question is why should anyone want to do that or if it is indeed correct using it for that purpose. Hence your commit.

We can't change files of ${D} in the do_deploy task and so
we need to move it to the do_install task.

This new UnLock efi is only required in the target and this
is the reason of the class-target override.
Before it don't fail on the class native because the do_deploy
only runs for the class target.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
Instaed of faling with a python exception we can check if we have
all keys reqired and fail with a useful message if not.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
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.

2 participants