Add better arch support + support ARM runners#178
Conversation
bleggett
changed the title
sveith
left a comment
sveith
left a comment
There was a problem hiding this comment.
Thank you for taking care of implementing the aarch64 builds.
I have some minor suggestions around the way shell escaping is done in this area of the code.
| def quoted(text: str) -> str: | ||
| return '"%s"' % text |
There was a problem hiding this comment.
For the purpose of shell-safe quoting, please use the builtin shlex.quote() method rather than creating one here.
| archs_env = os.getenv("KERNEL_ARCHITECTURES", "") | ||
| arch_env = os.getenv("KERNEL_ARCH", "") | ||
| if archs_env: | ||
| root_kernel_archs = [a.strip() for a in archs_env.split(",") if a.strip()] | ||
| elif arch_env: | ||
| root_kernel_archs = [arch_env.strip()] |
There was a problem hiding this comment.
I'm not fully convinced that the convenience of using KERNEL_ARCH=aarch64 over KERNEL_ARCHITECTURES=aarch64 is worth the additional logic; not blocking though.
| quoted( | ||
| '%s@$(cat "image-id-%s-%s-%s")' % (tag, tag_version, flavor, actual_target) | ||
| ), | ||
| quoted('%s@$(cat "%s")' % (tagged, iidfile)), |
There was a problem hiding this comment.
This is not strictly a new issue with the changes you are making, but I find the existing quoted() function misleading in this context: it does not quote its input in a way that prevents shell expansion (in fact, it looks like we deliberately want shell expansion here since we are doing code generation), and it is also not suitable for nesting.
This expression should be shlex.quote(tagged) + '@"$(cat %s)"' % shlex.quote(iidfile)
| if is_publish_enabled(): | ||
| # Start fresh so a re-run within the same workspace doesn't pick up | ||
| # digests from a previous (failed) invocation. | ||
| lines += ['rm -f "%s"' % DIGESTS_FILE] |
There was a problem hiding this comment.
If we're using quoted() or shlex.quote() elsewhere in this script, we should be doing so here as well (preferably the latter function).
2 participants
archoverrides, so specific flavors can build with uniquearchcombos (e.g.zone-nvidiagpu=x86_64only,zone=x86_64+aarch64, etc)QEMU+edera-largecrosscomp (experiment to see if this is actually faster - EDIT: as seen in CI it's equivalent to a coldcache build onedera-large, good enough)archto the matrix buildscript args:./hack/build/docker-build.sh "only-latest-lts:flavor=zone;arch=aarch64"zone(standard) kernel for bothx86_64andaarch64, so we don't have to choose between cross-building everything or nothing, and can be selective about it.