[PATCH 1/2] global: Do not default to faking missing binaries for buildman
While it is possible and documented on how to re-run buildman to replace faked required binary files after the fact, this behavior ends up being more confusing than helpful in practice. Switch to requiring BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this behavior.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile index 45f10759a137..535e52443ebf 100644 --- a/Makefile +++ b/Makefile @@ -1345,8 +1345,8 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \ $(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \ --toolpath $(objtree)/tools \ $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \ - build -u -d u-boot.dtb -O . -m --allow-missing \ - --fake-ext-blobs \ + build -u -d u-boot.dtb -O . -m \ + $(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \ -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com --- .azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0; - tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?; + tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 ]]; then tools/buildman/buildman -o /tmp -seP ${BUILDMAN}; exit $ret; diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7052a6061cb3..d9fb6518a0af 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -81,7 +81,7 @@ build all 32bit ARM platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E -W arm -x aarch64 || ret=$?; + ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries arm -x aarch64 || ret=$?; if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; @@ -94,7 +94,7 @@ build all 64bit ARM platforms: - . /tmp/venv/bin/activate - pip install pyelftools - ret=0; - ./tools/buildman/buildman -o /tmp -P -E -W aarch64 || ret=$?; + ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries aarch64 || ret=$?; if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; @@ -114,7 +114,7 @@ build all other platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E -W -x arm,powerpc || ret=$?; + ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries -x arm,powerpc || ret=$?; if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 76252b90792a..c07e2caab9a3 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -252,7 +252,8 @@ class Builder: mrproper=False, per_board_out_dir=False, config_only=False, squash_config_y=False, warnings_as_errors=False, work_in_output=False, - test_thread_exceptions=False, adjust_cfg=None): + test_thread_exceptions=False, adjust_cfg=None, + allow_missing_binaries=False): """Create a new Builder object
Args: @@ -290,6 +291,7 @@ class Builder: ~C to disable C C=val to set the value of C (val must have quotes if C is a string Kconfig + allow_missing_binaries: Run build with BINMAN_ALLOW_MISSING=1
""" self.toolchains = toolchains @@ -327,6 +329,7 @@ class Builder: self.config_filenames = BASE_CONFIG_FILENAMES self.work_in_output = work_in_output self.adjust_cfg = adjust_cfg + self.allow_missing_binaries = allow_missing_binaries self._ide = False
if not self.squash_config_y: diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 6240e08c7670..722ee17f0a03 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -253,6 +253,8 @@ class BuilderThread(threading.Thread): args.extend(['-j', str(self.builder.num_jobs)]) if self.builder.warnings_as_errors: args.append('KCFLAGS=-Werror') + if self.builder.allow_missing_binaries: + args.append('BINMAN_ALLOW_MISSING=1') config_args = ['%s_defconfig' % brd.target] config_out = '' args.extend(self.builder.toolchains.GetMakeArguments(brd)) diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index b29c1eb5ee72..18ef1902c7cd 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -15,6 +15,9 @@ def ParseArgs(): parser = OptionParser() parser.add_option('-a', '--adjust-cfg', type=str, action='append', help='Adjust the Kconfig settings in .config before building') + parser.add_option('--allow-missing-binaries', action='store_true', + default=False, help='Tell binman to allow for external binaries to' + ' be missing and generate fake ones as needed'), parser.add_option('-A', '--print-prefix', action='store_true', help='Print the tool-chain prefix for a board (CROSS_COMPILE=)') parser.add_option('-b', '--branch', type='string', diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 0c75466fbd3d..1289daeaef7e 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -329,7 +329,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, brds=None, warnings_as_errors=options.warnings_as_errors, work_in_output=options.work_in_output, test_thread_exceptions=test_thread_exceptions, - adjust_cfg=adjust_cfg) + adjust_cfg=adjust_cfg, + allow_missing_binaries=options.allow_missing_binaries) builder.force_config_on_failure = not options.quick if make_func: builder.do_make = make_func
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
This is fine for CI.
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
Regards, Simon
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make. Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do. Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
Hi Tom,
On Tue, 11 Oct 2022 at 10:22, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make.
OK. With the new -I/--ide flag I use buildman directly for single builds on some machines at least.
Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do.
I don't know, but I always use buildman directly for build-testing multiple boards.
Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
Yes I think that would work, although I think we would need two flags, one for building current source (where people might want it off) and another for building multiple boards (when presumably you want it on).
...although I don't see the advantage of this approach over the series I sent. Basically, the build should fail if there are missing blobs. The only question is whether we want to handle missing blobs by:
1. failing immediately when we see the first missing blob (your approach) 2. failing at the end after all binman processing is done (my approach)
The nice thing about 2 is that the build does complete and the exit code lets buildman decide what to do afterwards (i.e. we can make missing blobs be an error or a warning).
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
BTW we need a one-character flag if we do this as it will be a very common requirement.
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
I don't think people will be able to tell that. The 'make' fails with an error so it looks like a failure. It happens to be at the end in the binman step, but it is still a build failure.
My overall concern is constantly having to rerun a build because I forgot to add a flag, when I don't have the blobs for the boards anyway. Like the LTO thing that cost 4 seconds on every build, that could get really annoying. Blobs should not get in the way of development.
Regards, Simon
On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 10:22, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make.
OK. With the new -I/--ide flag I use buildman directly for single builds on some machines at least.
Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do.
I don't know, but I always use buildman directly for build-testing multiple boards.
I've been building for single machines with buildman for years, with --keep being passed to my wrapper script. And always use it for multiple boards. But this is you and I, to borrow a meme, we're builders georg and are outliers who should perhaps not be counted.
Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
Yes I think that would work, although I think we would need two flags, one for building current source (where people might want it off) and another for building multiple boards (when presumably you want it on).
I'm not sure. It's not "always use fake binaries" it's allow them. I'm honestly not sure how to tell buildman to use external binaries correctly, that's one of the cases where I use a different script that just manages output directories and known-good additional binaries.
...although I don't see the advantage of this approach over the series I sent. Basically, the build should fail if there are missing blobs. The only question is whether we want to handle missing blobs by:
- failing immediately when we see the first missing blob (your approach)
- failing at the end after all binman processing is done (my approach)
The nice thing about 2 is that the build does complete and the exit code lets buildman decide what to do afterwards (i.e. we can make missing blobs be an error or a warning).
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
The way I'm thinking of it is there's two cases. The first case is someone who is testing on the hardware that requires these files. Stop ASAP because they either will know they forgot to pass X/Y/Z files or they'll re-read the board instructions page and see oh, they need to grab binaries X/Y/Z too. Waiting to collect up missing file errors doesn't save time. It's also still hugely likely I believe that they're using make and not buildman.
The second case is someone building multiple boards at once to build-check (but not run-time check) something in multiple places. This is where passing a specific failure exit code can be helpful, yes.
BTW we need a one-character flag if we do this as it will be a very common requirement.
Sadly both -a and -A are taken. We could use -M for Missing ?
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
I don't think people will be able to tell that. The 'make' fails with an error so it looks like a failure. It happens to be at the end in the binman step, but it is still a build failure.
My overall concern is constantly having to rerun a build because I forgot to add a flag, when I don't have the blobs for the boards anyway. Like the LTO thing that cost 4 seconds on every build, that could get really annoying. Blobs should not get in the way of development.
The counter problem is this isn't the first time someone has come up and noted how much time they've wasted because we defaulted to fake binaries. I think we've optimized too much for the people that build a thousand configs all the time (us) instead of the people that build for 1 or two configs at a time (most people?).
To that end, I am really curious what Rasmus has to say here, or anyone else that has a different workflow from you and I.
On 12/10/2022 16.52, Tom Rini wrote:
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
The way I'm thinking of it is there's two cases. The first case is someone who is testing on the hardware that requires these files. Stop ASAP because they either will know they forgot to pass X/Y/Z files or they'll re-read the board instructions page and see oh, they need to grab binaries X/Y/Z too. Waiting to collect up missing file errors doesn't save time.
Indeed. If I forgot to put lpddr4_pmu_train_1d_dmem_202006.bin in my build folder, it is extremely unlikely I wouldn't remember to do the other three lpddr*.bin files at the same time. And even if there's some fifth file I also forgot, re-running make (yes, make) after putting the lpddr*.bin files in place doesn't take long since the whole tree is already built. I don't think any board really needs more than a handful of blobs (hopefully), and certainly not more than from a few sources (i.e., I count the imx8 lpddr*.bin as one), so there's really practically not much difference between "stop as soon as one is missing" or "give an error at the end and print all the missing stuff".
Personally, I usually prefer the "stop at first fatal error" model, because that makes it easier to see the actual problem - some of the follow-on errors/warnings could be due the first problem, and sometimes not stopping on that first error means the same problem gets printed over and over, making it hard to scroll back to find the first occurrence. Somewhat hand-wavy, yes, and I can't give any good examples.
The counter problem is this isn't the first time someone has come up and noted how much time they've wasted because we defaulted to fake binaries. I think we've optimized too much for the people that build a thousand configs all the time (us) instead of the people that build for 1 or two configs at a time (most people?).
To that end, I am really curious what Rasmus has to say here, or anyone else that has a different workflow from you and I.
Indeed my workflow never involves building a thousand configs, I leave that to upstream's CI.
I have roughly four different ways of building, all of them must fail if the resulting binary is known to be unusable (and I think we all agree on that part), and preferably without having to pass special flags to however the build is done (i.e., failing must be default, and I also think we're in agreement there), because otherwise I know those flags would be missed sometimes. Just to enumerate:
(1) I do local development, building in my xterm, and testing on target either by booting the binary with uuu or (for some other boards/SOCs) scp'ing it to target or however it can easily be deployed and tested.
(2) Once this has stabilized, I update our bitbake metadata to point at the new branch/commit, and do a local build with yocto (which is always done inside a docker container based on a specific image that we and our customers agree on using). That primarily catches things like missing host tools or libraries that I may happen to have on my dev machine but which are not in the docker image, or not exposed by Yocto. This can then either mean the recipe needs to grow a new DEPENDS, or (which is thankfully pretty rare) our docker image definition needs to be updated.
(3) When I can build with Yocto, it's time for my customer's CI to chew on it. Depending on the project, that sometimes involves automatically deploying the new bootloader to a test rack - which is why it's so important that we do not pass a build if the binary is known-broken.
(4) [And we're not here yet, but pretty close, which is why I've been rather active pushing stuff upstream the past few months] We want to have a CI job set up for automatically merging upstream master into our private branch, at least every -rc release, build test that and if successful, deploy it to target; if not (or if the merge cannot be done automatically in the first place), send an email so we're aware of it before the next release happens. So far, I've found three bugs in v2022.10 that could have been avoided (i.e. fixed before release) if we/I had this in place.
Since I'm writing this wall of text anyway, let me explain how I was bitten by the build not failing: I had added a new blob requirement (not a "proprietary" thing, just changing the logic so that the boot script is included in the u-boot fit image as a "loadable" and thus automatically loaded to a known location by SPL, instead of having U-Boot itself load it from somewhere), and locally added that bootscript.itb to my build folder when testing; I had also duly updated the bitbake recipe to copy bootscript.itb to "${B}" before do_compile, but failed to remember that that was not the right place to put it because the actual build folder is "${B}/<config name>". My own yocto build succeeded, I deployed the binary to the board on my desk, and it didn't work... Only then did I go look in do_compile.log and found the warning(s).
Rasmus
Hi Tom,
On Wed, 12 Oct 2022 at 08:52, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 10:22, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make.
OK. With the new -I/--ide flag I use buildman directly for single builds on some machines at least.
Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do.
I don't know, but I always use buildman directly for build-testing multiple boards.
I've been building for single machines with buildman for years, with --keep being passed to my wrapper script. And always use it for multiple boards. But this is you and I, to borrow a meme, we're builders georg and are outliers who should perhaps not be counted.
Yes, but we need to preserve that workflow.
Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
Yes I think that would work, although I think we would need two flags, one for building current source (where people might want it off) and another for building multiple boards (when presumably you want it on).
I'm not sure. It's not "always use fake binaries" it's allow them. I'm honestly not sure how to tell buildman to use external binaries correctly, that's one of the cases where I use a different script that just manages output directories and known-good additional binaries.
To me this is a policy thing. I just don't want to get 50 boards in and find that the builds start failing due to annoying blobs, or have it fail on the first blob when there is a binman error beyond just the first missing blob that is now masked.
...although I don't see the advantage of this approach over the series I sent. Basically, the build should fail if there are missing blobs. The only question is whether we want to handle missing blobs by:
- failing immediately when we see the first missing blob (your approach)
- failing at the end after all binman processing is done (my approach)
The nice thing about 2 is that the build does complete and the exit code lets buildman decide what to do afterwards (i.e. we can make missing blobs be an error or a warning).
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
The way I'm thinking of it is there's two cases. The first case is someone who is testing on the hardware that requires these files. Stop ASAP because they either will know they forgot to pass X/Y/Z files or they'll re-read the board instructions page and see oh, they need to grab binaries X/Y/Z too. Waiting to collect up missing file errors doesn't save time. It's also still hugely likely I believe that they're using make and not buildman.
The second case is someone building multiple boards at once to build-check (but not run-time check) something in multiple places. This is where passing a specific failure exit code can be helpful, yes.
Yes that makes sense.
BTW we need a one-character flag if we do this as it will be a very common requirement.
Sadly both -a and -A are taken. We could use -M for Missing ?
SGTM.
If we apply some of the patches in my series then I think I could enable that always, since the build will return an exit code if a blob is missing, which I can disable with -W.
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
I don't think people will be able to tell that. The 'make' fails with an error so it looks like a failure. It happens to be at the end in the binman step, but it is still a build failure.
My overall concern is constantly having to rerun a build because I forgot to add a flag, when I don't have the blobs for the boards anyway. Like the LTO thing that cost 4 seconds on every build, that could get really annoying. Blobs should not get in the way of development.
The counter problem is this isn't the first time someone has come up and noted how much time they've wasted because we defaulted to fake binaries. I think we've optimized too much for the people that build a thousand configs all the time (us) instead of the people that build for 1 or two configs at a time (most people?).
To that end, I am really curious what Rasmus has to say here, or anyone else that has a different workflow from you and I.
The main problem I think is that 'binman' should have emitted an exit code if there are missing blobs. That is [1] and I think was just an oversight on my part, or not thinking through things clearly enough.
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20221011141541.538175-5-sjg...
On Fri, Oct 14, 2022 at 09:56:40AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 12 Oct 2022 at 08:52, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 10:22, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
Add a new flag to buildman so that we will in turn pass BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
.azure-pipelines.yml | 2 +- .gitlab-ci.yml | 6 +++--- tools/buildman/builder.py | 5 ++++- tools/buildman/builderthread.py | 2 ++ tools/buildman/cmdline.py | 3 +++ tools/buildman/control.py | 3 ++- 6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index f200b40dbb24..c932c2b3c619 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -553,7 +553,7 @@ stages: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0;
tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make.
OK. With the new -I/--ide flag I use buildman directly for single builds on some machines at least.
Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do.
I don't know, but I always use buildman directly for build-testing multiple boards.
I've been building for single machines with buildman for years, with --keep being passed to my wrapper script. And always use it for multiple boards. But this is you and I, to borrow a meme, we're builders georg and are outliers who should perhaps not be counted.
Yes, but we need to preserve that workflow.
Right.
Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
Yes I think that would work, although I think we would need two flags, one for building current source (where people might want it off) and another for building multiple boards (when presumably you want it on).
I'm not sure. It's not "always use fake binaries" it's allow them. I'm honestly not sure how to tell buildman to use external binaries correctly, that's one of the cases where I use a different script that just manages output directories and known-good additional binaries.
To me this is a policy thing. I just don't want to get 50 boards in and find that the builds start failing due to annoying blobs, or have it fail on the first blob when there is a binman error beyond just the first missing blob that is now masked.
Yeah, there's some policy here. But managing policy is what config files are for too, right? Heck, I could probably drop some of my wrapper script and move more of it to the config file if I was confident in the syntax / sections. Maybe that needs to be documented?
...although I don't see the advantage of this approach over the series I sent. Basically, the build should fail if there are missing blobs. The only question is whether we want to handle missing blobs by:
- failing immediately when we see the first missing blob (your approach)
- failing at the end after all binman processing is done (my approach)
The nice thing about 2 is that the build does complete and the exit code lets buildman decide what to do afterwards (i.e. we can make missing blobs be an error or a warning).
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
The way I'm thinking of it is there's two cases. The first case is someone who is testing on the hardware that requires these files. Stop ASAP because they either will know they forgot to pass X/Y/Z files or they'll re-read the board instructions page and see oh, they need to grab binaries X/Y/Z too. Waiting to collect up missing file errors doesn't save time. It's also still hugely likely I believe that they're using make and not buildman.
The second case is someone building multiple boards at once to build-check (but not run-time check) something in multiple places. This is where passing a specific failure exit code can be helpful, yes.
Yes that makes sense.
BTW we need a one-character flag if we do this as it will be a very common requirement.
Sadly both -a and -A are taken. We could use -M for Missing ?
SGTM.
If we apply some of the patches in my series then I think I could enable that always, since the build will return an exit code if a blob is missing, which I can disable with -W.
I think so?
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
I don't think people will be able to tell that. The 'make' fails with an error so it looks like a failure. It happens to be at the end in the binman step, but it is still a build failure.
My overall concern is constantly having to rerun a build because I forgot to add a flag, when I don't have the blobs for the boards anyway. Like the LTO thing that cost 4 seconds on every build, that could get really annoying. Blobs should not get in the way of development.
The counter problem is this isn't the first time someone has come up and noted how much time they've wasted because we defaulted to fake binaries. I think we've optimized too much for the people that build a thousand configs all the time (us) instead of the people that build for 1 or two configs at a time (most people?).
To that end, I am really curious what Rasmus has to say here, or anyone else that has a different workflow from you and I.
The main problem I think is that 'binman' should have emitted an exit code if there are missing blobs. That is [1] and I think was just an oversight on my part, or not thinking through things clearly enough.
OK, can you take / use as inspiration what you think makes sense of my series as well and v2 your series? I want to grab that "N:" patch asap since that unblocks a bunch of other platforms that are ready to come in, otherwise.
Hi Tom,
On Fri, 14 Oct 2022 at 10:44, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 14, 2022 at 09:56:40AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 12 Oct 2022 at 08:52, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 10:22, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote: > > Add a new flag to buildman so that we will in turn pass > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI. > > Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk > Cc: Simon Glass sjg@chromium.org > Signed-off-by: Tom Rini trini@konsulko.com > --- > .azure-pipelines.yml | 2 +- > .gitlab-ci.yml | 6 +++--- > tools/buildman/builder.py | 5 ++++- > tools/buildman/builderthread.py | 2 ++ > tools/buildman/cmdline.py | 3 +++ > tools/buildman/control.py | 3 ++- > 6 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml > index f200b40dbb24..c932c2b3c619 100644 > --- a/.azure-pipelines.yml > +++ b/.azure-pipelines.yml > @@ -553,7 +553,7 @@ stages: > cat << "EOF" >> build.sh > if [[ "${BUILDMAN}" != "" ]]; then > ret=0; > - tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?; > + tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
This is fine for CI.
I think one of the issues here is we need to agree on the common use cases. I don't think most people use buildman to build, they use make.
OK. With the new -I/--ide flag I use buildman directly for single builds on some machines at least.
Of the people that do use buildman, how many aren't already using a wrapper script? I know I always do.
I don't know, but I always use buildman directly for build-testing multiple boards.
I've been building for single machines with buildman for years, with --keep being passed to my wrapper script. And always use it for multiple boards. But this is you and I, to borrow a meme, we're builders georg and are outliers who should perhaps not be counted.
Yes, but we need to preserve that workflow.
Right.
Can we also not just deal with setting this flag in ~/.buildman ? Would it Just Work as: [builder] allow-missing-binaries = True
Or is more code needed?
Yes I think that would work, although I think we would need two flags, one for building current source (where people might want it off) and another for building multiple boards (when presumably you want it on).
I'm not sure. It's not "always use fake binaries" it's allow them. I'm honestly not sure how to tell buildman to use external binaries correctly, that's one of the cases where I use a different script that just manages output directories and known-good additional binaries.
To me this is a policy thing. I just don't want to get 50 boards in and find that the builds start failing due to annoying blobs, or have it fail on the first blob when there is a binman error beyond just the first missing blob that is now masked.
Yeah, there's some policy here. But managing policy is what config files are for too, right? Heck, I could probably drop some of my wrapper script and move more of it to the config file if I was confident in the syntax / sections. Maybe that needs to be documented?
...although I don't see the advantage of this approach over the series I sent. Basically, the build should fail if there are missing blobs. The only question is whether we want to handle missing blobs by:
- failing immediately when we see the first missing blob (your approach)
- failing at the end after all binman processing is done (my approach)
The nice thing about 2 is that the build does complete and the exit code lets buildman decide what to do afterwards (i.e. we can make missing blobs be an error or a warning).
Option 1 has the benefit that we don't do any of the blob handling, so it just dies right away. Perhaps this is a philosophical question, but it is a little subtle and I'm not actually sure people would notice the difference so long as they get the errors as expected.
The way I'm thinking of it is there's two cases. The first case is someone who is testing on the hardware that requires these files. Stop ASAP because they either will know they forgot to pass X/Y/Z files or they'll re-read the board instructions page and see oh, they need to grab binaries X/Y/Z too. Waiting to collect up missing file errors doesn't save time. It's also still hugely likely I believe that they're using make and not buildman.
The second case is someone building multiple boards at once to build-check (but not run-time check) something in multiple places. This is where passing a specific failure exit code can be helpful, yes.
Yes that makes sense.
BTW we need a one-character flag if we do this as it will be a very common requirement.
Sadly both -a and -A are taken. We could use -M for Missing ?
SGTM.
If we apply some of the patches in my series then I think I could enable that always, since the build will return an exit code if a blob is missing, which I can disable with -W.
I think so?
But having to provide a flag to do build testing seems like the tail is wagging the dog. Boards should be discouraged to use blobs and we don't want to make it hard for everyone else (who doesn't have the blobs) to test whether their patch breaks a build.
I'm not sure this ends up being a common case. If someone is build testing for something they can't run test, the error is going to be after whatever they compile-tested was compiled/linked, and if they're testing everything it's via CI.
I don't think people will be able to tell that. The 'make' fails with an error so it looks like a failure. It happens to be at the end in the binman step, but it is still a build failure.
My overall concern is constantly having to rerun a build because I forgot to add a flag, when I don't have the blobs for the boards anyway. Like the LTO thing that cost 4 seconds on every build, that could get really annoying. Blobs should not get in the way of development.
The counter problem is this isn't the first time someone has come up and noted how much time they've wasted because we defaulted to fake binaries. I think we've optimized too much for the people that build a thousand configs all the time (us) instead of the people that build for 1 or two configs at a time (most people?).
To that end, I am really curious what Rasmus has to say here, or anyone else that has a different workflow from you and I.
The main problem I think is that 'binman' should have emitted an exit code if there are missing blobs. That is [1] and I think was just an oversight on my part, or not thinking through things clearly enough.
OK, can you take / use as inspiration what you think makes sense of my series as well and v2 your series? I want to grab that "N:" patch asap since that unblocks a bunch of other platforms that are ready to come in, otherwise.
Yes will do this by EOW.
Regards, Simon
Hi Tom,
On Mon, 10 Oct 2022 at 09:18, Tom Rini trini@konsulko.com wrote:
While it is possible and documented on how to re-run buildman to replace faked required binary files after the fact, this behavior ends up being more confusing than helpful in practice. Switch to requiring BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this behavior.
Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile index 45f10759a137..535e52443ebf 100644 --- a/Makefile +++ b/Makefile @@ -1345,8 +1345,8 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \ $(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \ --toolpath $(objtree)/tools \ $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
build -u -d u-boot.dtb -O . -m --allow-missing \--fake-ext-blobs \
build -u -d u-boot.dtb -O . -m \$(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \ -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ $(foreach f,$(BINMAN_INDIRS),-I $(f)) \-- 2.25.1
I'm not sure about this patch, but it might be good.
Basically the difference between this and the series I sent is that here, binman is told to fail as soon as it sees a missing blob, rather than finishing the image build and then failing.
So, for my approach, we get to see all the missing blobs and can presumably deal with them together
With this approach, we need to do them one at a time, but a benefit is that we do actually get that immediate failure and no messages about missing or faked blobs.
There are pros and cons.
Regards, Simon
On 10/10/2022 17.18, Tom Rini wrote:
While it is possible and documented on how to re-run buildman to replace faked required binary files after the fact, this behavior ends up being more confusing than helpful in practice. Switch to requiring BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this behavior.
Recreating the bitbake scenario that bit me with this applied correctly failed to build. Thanks.
Tested-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
Hi Rasmus,
On Tue, 11 Oct 2022 at 00:41, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
On 10/10/2022 17.18, Tom Rini wrote:
While it is possible and documented on how to re-run buildman to replace faked required binary files after the fact, this behavior ends up being more confusing than helpful in practice. Switch to requiring BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this behavior.
Recreating the bitbake scenario that bit me with this applied correctly failed to build. Thanks.
Tested-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
If you have time, can you try this one instead (or the series):
https://patchwork.ozlabs.org/project/uboot/patch/20221010200032.73483-5-sjg@...
Regards, Simon
participants (3)
-
Rasmus Villemoes -
Simon Glass -
Tom Rini