update wolfboot for option of using local wolfssl source and add wolfhsm recipe - #174
update wolfboot for option of using local wolfssl source and add wolfhsm recipe#174JacobBarthelmeh wants to merge 4 commits into
Conversation
|
Retest this please Jenkins |
night1rider
left a comment
night1rider
left a comment
There was a problem hiding this comment.
Some things I found. Biggest issues are with the newer syntax/variables related to the wrynose lts.
| # A plain download URL and a gs:// path are handled identically here: both | ||
| # are just a URI bitbake's fetcher understands. COMMERCIAL_BUNDLE_URL takes | ||
| # precedence so a recipe can offer the public download as its default while | ||
| # still allowing a GCS override. |
There was a problem hiding this comment.
Comment says GCS can still override, but url or gcs_uri means a non-empty URL wins and gcs_uri is never read. Might be good to comment that if both are set GCS is ignored.
There was a problem hiding this comment.
Reverting these wolfssl-commercial.bbclass changes. It is a bit of scope creep.
| SRC_URI = "git://github.com/wolfSSL/wolfHSM.git;protocol=https;branch=main" | ||
| SRCREV ?= "4aeecb2c35686bd4daeb40b3537500d15a93aff9" | ||
|
|
||
| S = "${WORKDIR}/git" |
There was a problem hiding this comment.
Older Yocto releases unpacked source straight into WORKDIR, which is why ${WORKDIR}/git used to be right. Newer ones unpack into a subfolder called UNPACKDIR and name the checkout after the recipe, so that path no longer exists. wrynose is listed in LAYERSERIES_COMPAT, so this layer promises to work there, and oe-core now rejects this exact line: bitbake wolfhsm stops at do_unpack with "Recipes that set S = "${WORKDIR}/git" ... should remove that assignment". Scarthgap will not show this, because it has no UNPACKDIR at all. wolfssl, wolftpm, wolfboot and others already test for UNPACKDIR and handle both layouts.
|
|
||
| # Guard against a half-copied tree: the pipeline above reports only the | ||
| # extract side's exit status under a plain POSIX shell. | ||
| if [ ! -f "${WOLFBOOT_WOLFSSL_STAGED_SRC}/wolfcrypt/src/asn.c" ]; then |
There was a problem hiding this comment.
This catches a copy that produced nothing at all. It will not catch one that started fine and died halfway, disk filling up, or a permissions problem part way down. In that case asn.c already exists, the check passes, and the build carries on with an incomplete tree.
Not sure if this is something we want to account for.
| # are just a URI bitbake's fetcher understands. COMMERCIAL_BUNDLE_URL takes | ||
| # precedence so a recipe can offer the public download as its default while | ||
| # still allowing a GCS override. | ||
| remote_uri = d.getVar('COMMERCIAL_BUNDLE_URL') or gcs_uri |
There was a problem hiding this comment.
When BitBake cannot find a variable, it does not blank it out. It leaves the raw text ${FOO} sitting in the value. That text is not empty, so this test passes it through and it ends up in SRC_URI as if it were a real web address. I tried it and parsing stopped across the whole build with MalformedUrl. The function above already guards for this with not X.startswith('${'), so the same check here would fix it.
| # ${WORKDIR}, not ${UNPACKDIR}: the layer still supports pre-styhead | ||
| # releases (LAYERSERIES_COMPAT reaches back to sumo) where file:// SRC_URI | ||
| # entries unpack straight into ${WORKDIR}. | ||
| install -m 0644 ${WORKDIR}/wolfhsm.mk ${D}${datadir}/wolfhsm/wolfhsm.mk |
There was a problem hiding this comment.
On newer releases a file:// entry lands in ${UNPACKDIR}, not directly in ${WORKDIR}, so this path points somewhere the file is not. I confirmed it separately by fixing only S, which left this failing on its own with install: cannot stat '.../wolfhsm.mk': No such file or directory.
| # order the class resolves them: | ||
| # | ||
| # 1. WOLFSSL_SRC_DIRECTORY - an already-extracted source tree. No fetch. | ||
| # 2. WOLFSSL_SRC_URL - a URL bitbake downloads the archive from. The |
There was a problem hiding this comment.
wolfssl-linuxkm-fips-ready.bb still only understands the GCS path, a local directory, and an already-extracted folder. It never sets COMMERCIAL_BUNDLE_URL, so when a user sets WOLFSSL_SRC_URL it builds a file path with no directory and fails on the checksum lookup.
| if remote_uri and bundle_archive: | ||
| unpack_flag = ';unpack=false' if bundle_archive.endswith('.7z') else '' | ||
| sha_flag = f';sha256sum={bundle_sha}' if bundle_sha else '' | ||
| filename_flag = f';downloadfilename={bundle_archive}' | ||
| return f'{gcs_uri}{filename_flag}{unpack_flag}{sha_flag}' | ||
| return f'{remote_uri}{filename_flag}{unpack_flag}{sha_flag}' | ||
|
|
||
| bundle_dir = d.getVar('COMMERCIAL_BUNDLE_DIR') |
There was a problem hiding this comment.
This task exists to unpack password-protected .7z bundles by hand, which is why it wants a folder to look in. A plain .zip from a URL needs none of that, because BitBake has already downloaded and unpacked it. The task knows this at line 198, where it says "not a .7z, let BitBake handle it" and returns, but line 186 demands the folder first and stops the build before getting there. I built it: do_fetch succeeds, then COMMERCIAL_BUNDLE_DIR not set. Moving :198 above :186 fixes it and leaves the .7z path unchanged.
f9fcc5b to
8128ebd
Compare
night1rider
left a comment
night1rider
left a comment
There was a problem hiding this comment.
Some smaller things I noticed, also not sure if we should address this issue in this PR since we are touching wolfBoot.
This looks to have been introduced when wolfBoot was added to meta-wolfssl:
bitbake world and bitbake -c cleanall world both fail with Nothing PROVIDES 'linux-xlnx', on scarthgap 5.0.19 and on wrynose 6.0.3, because line 23 sets KERNEL_PN ?= "linux-xlnx" and line 28 then does do_compile[depends] += "${KERNEL_PN}:do_deploy", so world always asks for a Xilinx kernel that most builds do not have. This is not new in this PR, I checked master and it fails the same way, but it is a wolfBoot recipe so it would be good to fix here. Adding EXCLUDE_FROM_WORLD = "1" to the recipe fixes it, and I tested that: bitbake -c cleanall world then runs clean at 2400 tasks.
| h.update(entry.encode()) | ||
| return h.hexdigest() | ||
|
|
||
| WOLFBOOT_WOLFSSL_SRC_ID = "${@wolfboot_wolfssl_src_id(d)}" |
There was a problem hiding this comment.
WOLFBOOT_WOLFSSL_SRC_IDis computed but never reaches the task hashes, so editing the external tree does not rebuild wolfBoot, which is the exact thing the docstring says it prevents. I added a line towolfcrypt/src/asn.cand the ID moved from64ebda33…tod5d3a68e…, but do_compilestayed at9f1bfc66…anddo_stage_external_wolfsslstayed at9fb03023…, so a stale wolfboot.elfgets reused. The cause is thatvardepshashes a variable's unexpanded content, so${@wolfboot_wolfssl_src_id(d)}is only ever a constant string to it, and setting the value withpython () { d.setVar('WOLFBOOT_WOLFSSL_SRC_ID', wolfboot_wolfssl_src_id(d)) }plusBB_DONT_CACHE = "1"fixes it, which I tested, and both parts are needed. While you are in there, the walk at lines 63 to 65 prunes fewer file types than the copy does, so once this works a leftover.o` will rebuild wolfBoot over a file that is never copied.
| rm -f "${WOLFBOOT_WOLFSSL_STAGED_SRC}.tar" | ||
| tar -cf "${WOLFBOOT_WOLFSSL_STAGED_SRC}.tar" -C "${WOLFBOOT_WOLFSSL_SRC}" \ | ||
| --exclude=.git --exclude=.libs \ | ||
| --exclude='*.o' --exclude='*.lo' --exclude='*.a' --exclude='*.la' \ | ||
| --exclude='*.so' --exclude='*.so.*' \ | ||
| . | ||
| tar -xf "${WOLFBOOT_WOLFSSL_STAGED_SRC}.tar" -C "${WOLFBOOT_WOLFSSL_STAGED_SRC}" | ||
| rm -f "${WOLFBOOT_WOLFSSL_STAGED_SRC}.tar" |
There was a problem hiding this comment.
Doing the tar in two steps is a good fix, but when it fails set -e stops the task before the rm -f on line 122 ever runs. I measured a 161,464,320 byte wolfssl-external.tar left behind in ${WORKDIR}, and the failure shows up as a bare ExecutionError(..., 2, None, None), so the reason is only in the task log. Wrapping each tar in if ! ...; then rm -f ...; bbfatal ...; fi covers both, and I tested it: the error then names the cause, the temp file is gone, and 0 files are staged.
| one `port/*/` directory directly into your application. This recipe makes that | ||
| possible from a Yocto build without vendoring a checkout. | ||
|
|
||
| ## Consuming it from a recipe |
There was a problem hiding this comment.
wolfHSM installs headers that #include <wolfssl/options.h>, but the recipe declares no DEPENDS, so a consumer following the README gets fatal error: wolfssl/options.h: No such file or directory. Adding DEPENDS = "wolfssl" here fixes it for every consumer, and I checked that the native and nativesdk variants still build. The consumer still has to configure wolfSSL with --enable-cryptocb --enable-keygen, and must not build with -std=c99, which is worth saying because upstream's own examples/posix Makefiles use exactly that.
There was a problem hiding this comment.
I see wolfBoot did not follow the convention when it was added, but should wolfHSM follow the existing naming instead? Every other recipe here is named for its release and pins with nobranch=1;rev=<sha>, for example wolftpm_4.1.0.bb and wolfssh_1.5.0.bb, while this one is wolfhsm_git.bb tracking branch=main.
No description provided.