ci: use specific variable for build-id - #3088
Jose Quaresma (quaresmajose) wants to merge 1 commit into
Conversation
Test Results 68 files - 51 319 suites - 396 8h 28m 58s ⏱️ - 7h 38m 14s For more details on these failures, see this check. Results for commit b277562. ± Comparison against base commit 7659417. This pull request removes 24 tests.This pull request skips 3 tests.♻️ This comment has been updated with latest results. |
Dmitry Baryshkov (lumag)
left a comment
Dmitry Baryshkov (lumag)
left a comment
There was a problem hiding this comment.
The variable BUILD_ID should be excluded from the dependencies.
Why? Commit messages should explain the reason for the changes.
The content of BUILD_ID is usually always different each time it's
runs or parsed. This happens because the variable contains the value
of DATETIME or the ${{ github.run_id }}-${{ github.run_attempt }}
github ci action. Therefore, we should rule out running any task
again if it is solely due to changes in the BUILD_ID.
The variable BUILD_ID should be excluded from the dependencies.
To do this more easily, the variable QCOM_BUILD_ID is introduced.
Signed-off-by: Jose Quaresma <jose.quaresma@oss.qualcomm.com>
44b6a3e to
b277562
Compare
I added it to the commit: The content of BUILD_ID is usually always different each time it's runs or parsed. This happens because the variable contains the value of DATETIME or the ${{ github.run_id }}-${{ github.run_attempt }} github ci action. Therefore, we should rule out running any task again if it is solely due to changes in the BUILD_ID. |
|
I have a strange feeling this will brake all LAVA jobs. Let me check more thoroughly. |
I believe this can be verified forcing a new ci rebuild/retest using the [Re-run all jobs] button of a ci job that has already been completed. |
|
I pushed this change to |
|
I think this is OK from testing perspective. The jobs are still running but they look OK. Example here: |
Thanks for checking it. |
| BUILD_ID ?= "local-${DATETIME}" | ||
| QCOM_BUILD_ID ?= "local-${DATETIME}" | ||
| BUILD_ID ?= "${QCOM_BUILD_ID}" | ||
| BUILD_ID[vardepsexclude] ?= "QCOM_BUILD_ID" |
There was a problem hiding this comment.
I don't get the change, this will result in builds reusing the build_id from the cache, which will produce a mismatch in os-release / uki / etc, and it goes against the main purpose of the change that added this.
There was a problem hiding this comment.
And some additional findings from AI:
- DATETIME exclusion is lost. BUILD_ID[vardepsexclude] ?= "QCOM_BUILD_ID" in local.conf is parsed before the recipe, so oe-core's ?= "DATETIME" becomes a no-op. Anyone setting BUILD_ID = "${DATETIME}-tag" (the upstream-documented pattern) now rebuilds os-release and every image on each invocation. Fix is "QCOM_BUILD_ID DATETIME", or drop the flag entirely.
- Both comments are now false. ci/base.yml lines 66-70 still say CI overrides BUILD_ID via the fragment, and compile.yml line 86 still promises os-release maps back to the S3 folder. Neither is true after this change.
- Weak binding regression. BUILD_ID ?= "${QCOM_BUILD_ID}" means any downstream hard BUILD_ID = unconditionally disconnects CI's run id. No in-tree fragment does this today.
There was a problem hiding this comment.
The idea behind this change is to enable the reuse of images from the sstate-cache. Something I've been cooking up for a while now in #2699
It may happen that the image in question does not correspond to the ${{ github.run_id }}-${{ github.run_attempt }} of the executed version in CI. But if this is the case, it's because the image met the conditions to be reused.
So for the execution AAAA-2 in CI we can have AAAA-1 inside the rootfs of the image. This means that the image was reused from ${{ github.run_attempt }}=1.
There was a problem hiding this comment.
I understand the idea, but while this will save build time and increase our reproducibility in our CI, it might also confuse users flashing our images for development and debugging purposes, as the runid won't necessarily match.
Unfortunately I don't really have a better solution as anything that touches the image will cause it to be created from scratch.
There was a problem hiding this comment.
I'm going to move this change to #2699 and leave it there while I implement the functionality.
5 participants
The variable BUILD_ID should be excluded from the dependencies. To do this more easily, the variable QCOM_BUILD_ID is introduced.
The content of BUILD_ID is usually always different each time it's runs or parsed. This happens because the variable contains the value of DATETIME or the ${{ github.run_id }}-${{ github.run_attempt }} github ci action. Therefore, we should rule out running any task again if it is solely due to changes in the BUILD_ID.