[PATCH 00/20] gitlab: Simplify the test script
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script: - Option to run a single build in a specified output directory - Allow ignoring warnings - Removes a restriction on the build output directory
It also - moves test.py over to use buildman for the --build option - makes one change to azure since the same approach should be possible there - fixes a few minor problems noticed in main and sandbox docs
Simon Glass (20): sandbox: Add documentation about required/useful packages main: Drop show_boot_progress() prototype buildman: Document the members of BuilderJob bulidman: Add support for a simple build gitlab: Use the -w option for sandbox_spl azure: Use the -w option for sandbox_spl gitlab: Use the --board buildman flag gitlab: Drop the BUILDMAN variable buildman: Update help for -d gitlab: Drop the buildman -d flag gitlab: Drop unnecessary if..fi gitlab: Use -w flag for all builds gitlab: Use bash to avoid needing a_test_which_does_not_exist buildman: Allow ignoring warnings in the return code gitlab: Use the buildman -W flag gitlab: Enable test_handoff buildman: Be more selective about which directories to remove buildman: Allow building within a subdir of the current dir test/py: Use buildman to build U-Boot gitlab: Simplify the exit code for test.py
.azure-pipelines.yml | 4 +- .gitlab-ci.yml | 81 +++++++++------------------------ common/main.c | 5 -- doc/arch/sandbox.rst | 10 ++++ test/py/conftest.py | 16 +++---- tools/buildman/README | 13 +++++- tools/buildman/builder.py | 51 ++++++++++++++++----- tools/buildman/builderthread.py | 34 ++++++++++---- tools/buildman/cmdline.py | 6 ++- tools/buildman/control.py | 35 ++++---------- tools/buildman/func_test.py | 46 ++++++++++++++----- tools/buildman/test.py | 20 ++++++++ 12 files changed, 186 insertions(+), 135 deletions(-)
Quite a few packages are used by sandbox or tools. Add a list of these to help people setting up for the first time.
Signed-off-by: Simon Glass sjg@chromium.org ---
doc/arch/sandbox.rst | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst index e577a95716..6a1c6fc552 100644 --- a/doc/arch/sandbox.rst +++ b/doc/arch/sandbox.rst @@ -34,6 +34,16 @@ integers can only be built on 64-bit hosts. Note that standalone/API support is not available at present.
+Prerequisites +------------- + +Here are some packages that are worth installing if you are doing sandbox or +tools development in U-Boot: + + python3-pytest lzma lzma-alone lz4 python3 python3-virtualenv + libssl1.0-dev + + Basic Operation ---------------
This is defined in bootstage.h and is not called in this file anyway. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/main.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/common/main.c b/common/main.c index ec8994ad45..06d7ff56d6 100644 --- a/common/main.c +++ b/common/main.c @@ -15,11 +15,6 @@ #include <init.h> #include <version.h>
-/* - * Board-specific Platform code can reimplement show_boot_progress () if needed - */ -__weak void show_boot_progress(int val) {} - static void run_preboot_environment_command(void) { char *p;
This class has a few more members now. Add documentation for them and fix a nit in the 'commits' comment.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/builderthread.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 570c1f6595..1e52ef8295 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -39,11 +39,15 @@ class BuilderJob:
Members: board: Board object to build - commits: List of commit options to build. + commits: List of Commit objects to build + keep_outputs: True to save build output files + step: 1 to process every commit, n to process every nth commit """ def __init__(self): self.board = None self.commits = [] + self.keep_outputs = False + self.step = 1
class ResultThread(threading.Thread):
It is useful to run a simple build and put all the output in a single directory. Add a -w option to support this.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/README | 11 ++++++++++ tools/buildman/builder.py | 15 +++++++++++-- tools/buildman/builderthread.py | 28 ++++++++++++++++------- tools/buildman/cmdline.py | 2 ++ tools/buildman/control.py | 10 ++++++++- tools/buildman/func_test.py | 39 +++++++++++++++++++++++++++++---- 6 files changed, 90 insertions(+), 15 deletions(-)
diff --git a/tools/buildman/README b/tools/buildman/README index c1ac0d0f58..abbbbea9f2 100644 --- a/tools/buildman/README +++ b/tools/buildman/README @@ -1056,6 +1056,17 @@ toolchain. For example: buildman -O clang-7 --board sandbox
+Doing a simple build +==================== + +In some cases you just want to build a single board and get the full output, use +the -w option, for example: + + buildman -o /tmp/build --board sandbox -w + +This will write the full build into /tmp/build including object files. + + Other options =============
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 3fd4fac695..081c1d0901 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -174,6 +174,8 @@ class Builder: in_tree: Build U-Boot in-tree instead of specifying an output directory separate from the source code. This option is really only useful for testing in-tree builds. + work_in_output: Use the output directory as the work directory and + don't write to a separate output directory.
Private members: _base_board_dict: Last-summarised Dict of boards @@ -224,7 +226,7 @@ class Builder: no_subdirs=False, full_path=False, verbose_build=False, incremental=False, per_board_out_dir=False, config_only=False, squash_config_y=False, - warnings_as_errors=False): + warnings_as_errors=False, work_in_output=False): """Create a new Builder object
Args: @@ -250,10 +252,15 @@ class Builder: config_only: Only configure each build, don't build it squash_config_y: Convert CONFIG options with the value 'y' to '1' warnings_as_errors: Treat all compiler warnings as errors + work_in_output: Use the output directory as the work directory and + don't write to a separate output directory. """ self.toolchains = toolchains self.base_dir = base_dir - self._working_dir = os.path.join(base_dir, '.bm-work') + if work_in_output: + self._working_dir = base_dir + else: + self._working_dir = os.path.join(base_dir, '.bm-work') self.threads = [] self.do_make = self.Make self.gnu_make = gnu_make @@ -280,6 +287,7 @@ class Builder: self.config_only = config_only self.squash_config_y = squash_config_y self.config_filenames = BASE_CONFIG_FILENAMES + self.work_in_output = work_in_output if not self.squash_config_y: self.config_filenames += EXTRA_CONFIG_FILENAMES
@@ -1474,6 +1482,8 @@ class Builder: Args: thread_num: Number of thread to check. """ + if self.work_in_output: + return self._working_dir return os.path.join(self._working_dir, '%02d' % thread_num)
def _PrepareThread(self, thread_num, setup_git): @@ -1571,6 +1581,7 @@ class Builder: job.board = brd job.commits = commits job.keep_outputs = keep_outputs + job.work_in_output = self.work_in_output job.step = self._step self.queue.put(job)
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 1e52ef8295..7561f39942 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -42,12 +42,15 @@ class BuilderJob: commits: List of Commit objects to build keep_outputs: True to save build output files step: 1 to process every commit, n to process every nth commit + work_in_output: Use the output directory as the work directory and + don't write to a separate output directory. """ def __init__(self): self.board = None self.commits = [] self.keep_outputs = False self.step = 1 + self.work_in_output = False
class ResultThread(threading.Thread): @@ -118,7 +121,7 @@ class BuilderThread(threading.Thread): **kwargs)
def RunCommit(self, commit_upto, brd, work_dir, do_config, config_only, - force_build, force_build_failures): + force_build, force_build_failures, work_in_output): """Build a particular commit.
If the build is already done, and we are not forcing a build, we skip @@ -133,6 +136,8 @@ class BuilderThread(threading.Thread): force_build: Force a build even if one was previously done force_build_failures: Force a bulid if the previous result showed failure + work_in_output: Use the output directory as the work directory and + don't write to a separate output directory.
Returns: tuple containing: @@ -143,7 +148,7 @@ class BuilderThread(threading.Thread): # self.Make() below, in the event that we do a build. result = command.CommandResult() result.return_code = 0 - if self.builder.in_tree: + if work_in_output or self.builder.in_tree: out_dir = work_dir else: if self.per_board_out_dir: @@ -265,14 +270,18 @@ class BuilderThread(threading.Thread): result.out_dir = out_dir return result, do_config
- def _WriteResult(self, result, keep_outputs): + def _WriteResult(self, result, keep_outputs, work_in_output): """Write a built result to the output directory.
Args: result: CommandResult object containing result to write keep_outputs: True to store the output binaries, False to delete them + work_in_output: Use the output directory as the work directory and + don't write to a separate output directory. """ + if work_in_output: + return # Fatal error if result.return_code < 0: return @@ -434,7 +443,8 @@ class BuilderThread(threading.Thread): result, request_config = self.RunCommit(commit_upto, brd, work_dir, do_config, self.builder.config_only, force_build or self.builder.force_build, - self.builder.force_build_failures) + self.builder.force_build_failures, + work_in_output=job.work_in_output) failed = result.return_code or result.stderr did_config = do_config if failed and not do_config: @@ -442,7 +452,8 @@ class BuilderThread(threading.Thread): # with a reconfig. if self.builder.force_config_on_failure: result, request_config = self.RunCommit(commit_upto, - brd, work_dir, True, False, True, False) + brd, work_dir, True, False, True, False, + work_in_output=job.work_in_output) did_config = True if not self.builder.force_reconfig: do_config = request_config @@ -481,15 +492,16 @@ class BuilderThread(threading.Thread): raise ValueError('Interrupt')
# We have the build results, so output the result - self._WriteResult(result, job.keep_outputs) + self._WriteResult(result, job.keep_outputs, job.work_in_output) self.builder.out_queue.put(result) else: # Just build the currently checked-out build result, request_config = self.RunCommit(None, brd, work_dir, True, self.builder.config_only, True, - self.builder.force_build_failures) + self.builder.force_build_failures, + work_in_output=job.work_in_output) result.commit_upto = 0 - self._WriteResult(result, job.keep_outputs) + self._WriteResult(result, job.keep_outputs, job.work_in_output) self.builder.out_queue.put(result)
def run(self): diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index b41209373d..c7b4bf6b4a 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -106,6 +106,8 @@ def ParseArgs(): default=False, help='Show build results while the build progresses') parser.add_option('-V', '--verbose-build', action='store_true', default=False, help='Run make with V=1, logging all output') + parser.add_option('-w', '--work-in-output', action='store_true', + default=False, help='Use the output directory as the work directory') parser.add_option('-x', '--exclude', dest='exclude', type='string', action='append', help='Specify a list of boards to exclude, separated by comma') diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 969d866547..5d80400f7a 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -263,6 +263,13 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, str = ("No commits found to process in branch '%s': " "set branch's upstream or use -c flag" % options.branch) sys.exit(col.Color(col.RED, str)) + if options.work_in_output: + if len(selected) != 1: + sys.exit(col.Color(col.RED, + '-w can only be used with a single board')) + if count != 1: + sys.exit(col.Color(col.RED, + '-w can only be used with a single commit'))
# Read the metadata from the commits. First look at the upstream commit, # then the ones in the branch. We would like to do something like @@ -334,7 +341,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, per_board_out_dir=options.per_board_out_dir, config_only=options.config_only, squash_config_y=not options.preserve_config_y, - warnings_as_errors=options.warnings_as_errors) + warnings_as_errors=options.warnings_as_errors, + work_in_output=options.work_in_output) builder.force_config_on_failure = not options.quick if make_func: builder.do_make = make_func diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index 4c3d497294..f9f8f80593 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -16,6 +16,7 @@ import control import gitutil import terminal import toolchain +import tools
settings_data = ''' # Buildman settings file @@ -208,7 +209,7 @@ class TestFunctional(unittest.TestCase):
def tearDown(self): shutil.rmtree(self._base_dir) - shutil.rmtree(self._output_dir) + #shutil.rmtree(self._output_dir)
def setupToolchains(self): self._toolchains = toolchain.Toolchains() @@ -218,12 +219,12 @@ class TestFunctional(unittest.TestCase): return command.RunPipe([[self._buildman_pathname] + list(args)], capture=True, capture_stderr=True)
- def _RunControl(self, *args, **kwargs): + def _RunControl(self, *args, clean_dir=False, boards=None): sys.argv = [sys.argv[0]] + list(args) options, args = cmdline.ParseArgs() result = control.DoBuildman(options, args, toolchains=self._toolchains, - make_func=self._HandleMake, boards=self._boards, - clean_dir=kwargs.get('clean_dir', True)) + make_func=self._HandleMake, boards=boards or self._boards, + clean_dir=clean_dir) self._builder = control.builder return result
@@ -397,6 +398,12 @@ class TestFunctional(unittest.TestCase): combined='Test configuration complete') elif stage == 'build': stderr = '' + out_dir = '' + for arg in args: + if arg.startswith('O='): + out_dir = arg[2:] + fname = os.path.join(cwd or '', out_dir, 'u-boot') + tools.WriteFile(fname, b'U-Boot') if type(commit) is not str: stderr = self._error.get((brd.target, commit.sequence)) if stderr: @@ -535,3 +542,27 @@ class TestFunctional(unittest.TestCase): with self.assertRaises(SystemExit): self._RunControl('-b', self._test_branch, '-o', os.path.join(os.getcwd(), 'test')) + + def testWorkInOutput(self): + """Test the -w option which should write directly to the output dir""" + board_list = board.Boards() + board_list.AddBoard(board.Board(*boards[0])) + self._RunControl('-o', self._output_dir, '-w', clean_dir=False, + boards=board_list) + self.assertTrue( + os.path.exists(os.path.join(self._output_dir, 'u-boot'))) + + def testWorkInOutputFail(self): + """Test the -w option failures""" + with self.assertRaises(SystemExit) as e: + self._RunControl('-o', self._output_dir, '-w', clean_dir=False) + self.assertIn("single board", str(e.exception)) + self.assertFalse( + os.path.exists(os.path.join(self._output_dir, 'u-boot'))) + + board_list = board.Boards() + board_list.AddBoard(board.Board(*boards[0])) + with self.assertRaises(SystemExit) as e: + self._RunControl('-b', self._test_branch, '-o', self._output_dir, + '-w', clean_dir=False, boards=board_list) + self.assertIn("single commit", str(e.exception))
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 55943bb3a2..248e0530d2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -167,10 +167,10 @@ Run binman, buildman, dtoc and patman testsuites: virtualenv -p /usr/bin/python3 /tmp/venv; . /tmp/venv/bin/activate; pip install pyelftools; - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/sandbox_spl; + export UBOOT_TRAVIS_BUILD_DIR=/tmp/sandbox_spl; export PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt"; export PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}"; - ./tools/buildman/buildman -o /tmp -P sandbox_spl; + ./tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w sandbox_spl; ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test; ./tools/buildman/buildman -t; ./tools/dtoc/dtoc -t;
On Fri, Mar 06, 2020 at 08:07:19PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
What I'd like to see is GitLab / Azure / Travis changing in-sync with these steps, rather than one commit for each (and Travis getting out of sync). Thanks!
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Signed-off-by: Simon Glass sjg@chromium.org ---
.azure-pipelines.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index 86480581a2..f44b196a15 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -136,10 +136,10 @@ jobs: virtualenv -p /usr/bin/python3 /tmp/venv . /tmp/venv/bin/activate pip install pyelftools - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/sandbox_spl + export UBOOT_TRAVIS_BUILD_DIR=/tmp/sandbox_spl export PYTHONPATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt export PATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH} - ./tools/buildman/buildman -o /tmp -P sandbox_spl + ./tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w sandbox_spl ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test ./tools/buildman/buildman -t ./tools/dtoc/dtoc -t
The current method of selecting the board to build is a bit error-prone, e.g. with "^sandbox$" it actually builds 5 boards (all of those in the sandbox architecture).
Use the (newish) --board flag instead, to get the same result.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 248e0530d2..3f48cad752 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -31,9 +31,10 @@ stages: # use clang only do one configuration. - if [[ "${BUILDMAN}" != "" ]]; then ret=0; - tools/buildman/buildman -o /tmp -P -E ${BUILDMAN} ${OVERRIDE}|| ret=$?; + tools/buildman/buildman -o /tmp -P -E --board ${BUILDMAN} ${OVERRIDE} + || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - tools/buildman/buildman -o /tmp -sdeP ${BUILDMAN}; + tools/buildman/buildman -o /tmp -sdeP --board ${BUILDMAN}; exit $ret; fi; fi @@ -181,14 +182,14 @@ sandbox test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox" - BUILDMAN: "^sandbox$" + BUILDMAN: "sandbox" <<: *buildman_and_testpy_dfn
sandbox with clang test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox" - BUILDMAN: "^sandbox$" + BUILDMAN: "sandbox" OVERRIDE: "-O clang-7" <<: *buildman_and_testpy_dfn
@@ -196,7 +197,7 @@ sandbox_spl test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_spl" - BUILDMAN: "^sandbox_spl$" + BUILDMAN: "sandbox_spl" TEST_PY_TEST_SPEC: "test_ofplatdata" <<: *buildman_and_testpy_dfn
@@ -205,14 +206,14 @@ evb-ast2500 test.py: variables: TEST_PY_BD: "evb-ast2500" TEST_PY_ID: "--id qemu" - BUILDMAN: "^evb-ast2500$" + BUILDMAN: "evb-ast2500" <<: *buildman_and_testpy_dfn
sandbox_flattree test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_flattree" - BUILDMAN: "^sandbox_flattree$" + BUILDMAN: "sandbox_flattree" <<: *buildman_and_testpy_dfn
vexpress_ca15_tc2 test.py: @@ -220,7 +221,7 @@ vexpress_ca15_tc2 test.py: variables: TEST_PY_BD: "vexpress_ca15_tc2" TEST_PY_ID: "--id qemu" - BUILDMAN: "^vexpress_ca15_tc2$" + BUILDMAN: "vexpress_ca15_tc2" <<: *buildman_and_testpy_dfn
vexpress_ca9x4 test.py: @@ -228,7 +229,7 @@ vexpress_ca9x4 test.py: variables: TEST_PY_BD: "vexpress_ca9x4" TEST_PY_ID: "--id qemu" - BUILDMAN: "^vexpress_ca9x4$" + BUILDMAN: "vexpress_ca9x4" <<: *buildman_and_testpy_dfn
integratorcp_cm926ejs test.py: @@ -237,7 +238,7 @@ integratorcp_cm926ejs test.py: TEST_PY_BD: "integratorcp_cm926ejs" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "^integratorcp_cm926ejs$" + BUILDMAN: "integratorcp_cm926ejs" <<: *buildman_and_testpy_dfn
qemu_arm test.py: @@ -245,7 +246,7 @@ qemu_arm test.py: variables: TEST_PY_BD: "qemu_arm" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_arm$" + BUILDMAN: "qemu_arm" <<: *buildman_and_testpy_dfn
qemu_arm64 test.py: @@ -253,7 +254,7 @@ qemu_arm64 test.py: variables: TEST_PY_BD: "qemu_arm64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_arm64$" + BUILDMAN: "qemu_arm64" <<: *buildman_and_testpy_dfn
qemu_mips test.py: @@ -261,7 +262,7 @@ qemu_mips test.py: variables: TEST_PY_BD: "qemu_mips" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_mips$" + BUILDMAN: "qemu_mips" <<: *buildman_and_testpy_dfn
qemu_mipsel test.py: @@ -269,7 +270,7 @@ qemu_mipsel test.py: variables: TEST_PY_BD: "qemu_mipsel" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_mipsel$" + BUILDMAN: "qemu_mipsel" <<: *buildman_and_testpy_dfn
qemu_mips64 test.py: @@ -277,7 +278,7 @@ qemu_mips64 test.py: variables: TEST_PY_BD: "qemu_mips64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_mips64$" + BUILDMAN: "qemu_mips64" <<: *buildman_and_testpy_dfn
qemu_mips64el test.py: @@ -285,7 +286,7 @@ qemu_mips64el test.py: variables: TEST_PY_BD: "qemu_mips64el" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu_mips64el$" + BUILDMAN: "qemu_mips64el" <<: *buildman_and_testpy_dfn
qemu-ppce500 test.py: @@ -293,7 +294,7 @@ qemu-ppce500 test.py: variables: TEST_PY_BD: "qemu-ppce500" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu-ppce500$" + BUILDMAN: "qemu-ppce500" <<: *buildman_and_testpy_dfn
qemu-riscv64 test.py: @@ -301,7 +302,7 @@ qemu-riscv64 test.py: variables: TEST_PY_BD: "qemu-riscv64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu-riscv64$" + BUILDMAN: "qemu-riscv64" <<: *buildman_and_testpy_dfn
qemu-x86 test.py: @@ -309,7 +310,7 @@ qemu-x86 test.py: variables: TEST_PY_BD: "qemu-x86" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu-x86$" + BUILDMAN: "qemu-x86" <<: *buildman_and_testpy_dfn
qemu-x86_64 test.py: @@ -317,7 +318,7 @@ qemu-x86_64 test.py: variables: TEST_PY_BD: "qemu-x86_64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "^qemu-x86_64$" + BUILDMAN: "qemu-x86_64" <<: *buildman_and_testpy_dfn
zynq_zc702 test.py: @@ -326,7 +327,7 @@ zynq_zc702 test.py: TEST_PY_BD: "zynq_zc702" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "^zynq_zc702$" + BUILDMAN: "zynq_zc702" <<: *buildman_and_testpy_dfn
xilinx_versal_virt test.py: @@ -335,7 +336,7 @@ xilinx_versal_virt test.py: TEST_PY_BD: "xilinx_versal_virt" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "^xilinx_versal_virt$" + BUILDMAN: "xilinx_versal_virt" <<: *buildman_and_testpy_dfn
xtfpga test.py: @@ -344,5 +345,5 @@ xtfpga test.py: TEST_PY_BD: "xtfpga" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "^xtfpga$" + BUILDMAN: "xtfpga" <<: *buildman_and_testpy_dfn
On Fri, Mar 06, 2020 at 08:07:21PM -0700, Simon Glass wrote:
The current method of selecting the board to build is a bit error-prone, e.g. with "^sandbox$" it actually builds 5 boards (all of those in the sandbox architecture).
OK, hold up. You've reminded me of something that's been broken for a while and I forgot to follow up on. Which is that at some point we stopped applying regex to either only the defconfig name, or stopped checking that as the first one. The example in question used to only build sandbox_defconfig, but now (and for some time) has built all of arch or soc sandbox.
That said, I'm not sure that at the end of the day I'm opposed to the --board flag instead.
Hi Tom,
On Mon, 9 Mar 2020 at 11:46, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:21PM -0700, Simon Glass wrote:
The current method of selecting the board to build is a bit error-prone, e.g. with "^sandbox$" it actually builds 5 boards (all of those in the sandbox architecture).
OK, hold up. You've reminded me of something that's been broken for a while and I forgot to follow up on. Which is that at some point we stopped applying regex to either only the defconfig name, or stopped checking that as the first one. The example in question used to only build sandbox_defconfig, but now (and for some time) has built all of arch or soc sandbox.
Are you sure we even used just the defconfig name? I don't recall that. Do you know when this was?
Since 'sandbox' is an arch name this will build anything sandbox board, and I think that is intended.
That said, I'm not sure that at the end of the day I'm opposed to the --board flag instead.
OK.
Regards, Simon
This is not needed now since we use the same name as the pytests.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3f48cad752..2cd6209222 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,12 +29,12 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration. - - if [[ "${BUILDMAN}" != "" ]]; then + - if [[ "${TEST_PY_BD}" != "" ]]; then ret=0; - tools/buildman/buildman -o /tmp -P -E --board ${BUILDMAN} ${OVERRIDE} + tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - tools/buildman/buildman -o /tmp -sdeP --board ${BUILDMAN}; + tools/buildman/buildman -o /tmp -sdeP --board ${TEST_PY_BD}; exit $ret; fi; fi @@ -182,14 +182,12 @@ sandbox test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox" - BUILDMAN: "sandbox" <<: *buildman_and_testpy_dfn
sandbox with clang test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox" - BUILDMAN: "sandbox" OVERRIDE: "-O clang-7" <<: *buildman_and_testpy_dfn
@@ -197,7 +195,6 @@ sandbox_spl test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_spl" - BUILDMAN: "sandbox_spl" TEST_PY_TEST_SPEC: "test_ofplatdata" <<: *buildman_and_testpy_dfn
@@ -206,14 +203,12 @@ evb-ast2500 test.py: variables: TEST_PY_BD: "evb-ast2500" TEST_PY_ID: "--id qemu" - BUILDMAN: "evb-ast2500" <<: *buildman_and_testpy_dfn
sandbox_flattree test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_flattree" - BUILDMAN: "sandbox_flattree" <<: *buildman_and_testpy_dfn
vexpress_ca15_tc2 test.py: @@ -221,7 +216,6 @@ vexpress_ca15_tc2 test.py: variables: TEST_PY_BD: "vexpress_ca15_tc2" TEST_PY_ID: "--id qemu" - BUILDMAN: "vexpress_ca15_tc2" <<: *buildman_and_testpy_dfn
vexpress_ca9x4 test.py: @@ -229,7 +223,6 @@ vexpress_ca9x4 test.py: variables: TEST_PY_BD: "vexpress_ca9x4" TEST_PY_ID: "--id qemu" - BUILDMAN: "vexpress_ca9x4" <<: *buildman_and_testpy_dfn
integratorcp_cm926ejs test.py: @@ -238,7 +231,6 @@ integratorcp_cm926ejs test.py: TEST_PY_BD: "integratorcp_cm926ejs" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "integratorcp_cm926ejs" <<: *buildman_and_testpy_dfn
qemu_arm test.py: @@ -246,7 +238,6 @@ qemu_arm test.py: variables: TEST_PY_BD: "qemu_arm" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_arm" <<: *buildman_and_testpy_dfn
qemu_arm64 test.py: @@ -254,7 +245,6 @@ qemu_arm64 test.py: variables: TEST_PY_BD: "qemu_arm64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_arm64" <<: *buildman_and_testpy_dfn
qemu_mips test.py: @@ -262,7 +252,6 @@ qemu_mips test.py: variables: TEST_PY_BD: "qemu_mips" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_mips" <<: *buildman_and_testpy_dfn
qemu_mipsel test.py: @@ -270,7 +259,6 @@ qemu_mipsel test.py: variables: TEST_PY_BD: "qemu_mipsel" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_mipsel" <<: *buildman_and_testpy_dfn
qemu_mips64 test.py: @@ -278,7 +266,6 @@ qemu_mips64 test.py: variables: TEST_PY_BD: "qemu_mips64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_mips64" <<: *buildman_and_testpy_dfn
qemu_mips64el test.py: @@ -286,7 +273,6 @@ qemu_mips64el test.py: variables: TEST_PY_BD: "qemu_mips64el" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu_mips64el" <<: *buildman_and_testpy_dfn
qemu-ppce500 test.py: @@ -294,7 +280,6 @@ qemu-ppce500 test.py: variables: TEST_PY_BD: "qemu-ppce500" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu-ppce500" <<: *buildman_and_testpy_dfn
qemu-riscv64 test.py: @@ -302,7 +287,6 @@ qemu-riscv64 test.py: variables: TEST_PY_BD: "qemu-riscv64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu-riscv64" <<: *buildman_and_testpy_dfn
qemu-x86 test.py: @@ -310,7 +294,6 @@ qemu-x86 test.py: variables: TEST_PY_BD: "qemu-x86" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu-x86" <<: *buildman_and_testpy_dfn
qemu-x86_64 test.py: @@ -318,7 +301,6 @@ qemu-x86_64 test.py: variables: TEST_PY_BD: "qemu-x86_64" TEST_PY_TEST_SPEC: "not sleep" - BUILDMAN: "qemu-x86_64" <<: *buildman_and_testpy_dfn
zynq_zc702 test.py: @@ -327,7 +309,6 @@ zynq_zc702 test.py: TEST_PY_BD: "zynq_zc702" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "zynq_zc702" <<: *buildman_and_testpy_dfn
xilinx_versal_virt test.py: @@ -336,7 +317,6 @@ xilinx_versal_virt test.py: TEST_PY_BD: "xilinx_versal_virt" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "xilinx_versal_virt" <<: *buildman_and_testpy_dfn
xtfpga test.py: @@ -345,5 +325,4 @@ xtfpga test.py: TEST_PY_BD: "xtfpga" TEST_PY_TEST_SPEC: "not sleep" TEST_PY_ID: "--id qemu" - BUILDMAN: "xtfpga" <<: *buildman_and_testpy_dfn
This help is a bit ambiguous. It only does anything if showing size changes with -S. Update the help and the function comments.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/builder.py | 6 +++--- tools/buildman/cmdline.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 081c1d0901..a41d0b316e 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -337,7 +337,7 @@ class Builder:
show_errors: True to show summarised error/warning info show_sizes: Show size deltas - show_detail: Show detail for each board + show_detail: Show size delta detail for each board if show_sizes show_bloat: Show detail for each function list_error_boards: Show the boards which caused each error/warning show_config: Show config deltas @@ -1000,7 +1000,7 @@ class Builder: board.target board_dict: Dict containing boards for which we built this commit, keyed by board.target. The value is an Outcome object. - show_detail: Show detail for each board + show_detail: Show size delta detail for each board show_bloat: Show detail for each function """ arch_list = {} @@ -1117,7 +1117,7 @@ class Builder: environment: Dictionary keyed by environment variable, Each value is the value of environment variable. show_sizes: Show image size deltas - show_detail: Show detail for each board + show_detail: Show size delta detail for each board if show_sizes show_bloat: Show detail for each function show_config: Show config changes show_environment: Show environment changes diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index c7b4bf6b4a..74b410010d 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -31,7 +31,7 @@ def ParseArgs(): help='Reconfigure for every commit (disable incremental build)') parser.add_option('-d', '--detail', dest='show_detail', action='store_true', default=False, - help='Show detailed information for each board in summary') + help='Show detailed size delta for each board in the -S summary') parser.add_option('-D', '--config-only', action='store_true', default=False, help="Don't build, just configure each commit") parser.add_option('-e', '--show_errors', action='store_true',
This has no effect since -S is not given also. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2cd6209222..36c2ecfa43 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -34,7 +34,7 @@ stages: tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - tools/buildman/buildman -o /tmp -sdeP --board ${TEST_PY_BD}; + tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fi; fi @@ -65,7 +65,7 @@ build all 32bit ARM platforms: - ret=0; ./tools/buildman/buildman -o /tmp -P -E arm -x aarch64 || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; + ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi;
@@ -79,7 +79,7 @@ build all 64bit ARM platforms: - ret=0; ./tools/buildman/buildman -o /tmp -P -E aarch64 || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; + ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi;
@@ -90,7 +90,7 @@ build all PowerPC platforms: - ret=0; ./tools/buildman/buildman -o /tmp -P -E powerpc || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; + ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi;
@@ -101,7 +101,7 @@ build all other platforms: - ret=0; ./tools/buildman/buildman -o /tmp -P -E -x arm,powerpc || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; + ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi;
Since TEST_PY_BD is always defined we can drop this check.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 36c2ecfa43..b29d59d942 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,14 +29,12 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration. - - if [[ "${TEST_PY_BD}" != "" ]]; then - ret=0; - tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} - || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; - exit $ret; - fi; + - ret=0; + tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} + || ret=$?; + if [[ $ret -ne 0 && $ret -ne 129 ]]; then + tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; + exit $ret; fi # "not a_test_which_does_not_exist" is a dummy -k parameter which will # never prevent any test from running. That way, we can always pass @@ -48,15 +46,13 @@ stages: - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/${TEST_PY_BD}; export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; - if [[ "${TEST_PY_BD}" != "" ]]; then - ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} - -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}" - --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; - ret=$?; - if [[ $ret -ne 0 ]]; then - exit $ret; - fi; - fi; + ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} + -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}" + --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; + ret=$?; + if [[ $ret -ne 0 ]]; then + exit $ret; + fi
build all 32bit ARM platforms: tags: [ 'all' ]
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration. + - export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD} - ret=0; - tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} - || ret=$?; + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E + --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then - tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fi # "not a_test_which_does_not_exist" is a dummy -k parameter which will @@ -43,8 +43,7 @@ stages: - virtualenv -p /usr/bin/python3 /tmp/venv - . /tmp/venv/bin/activate - pip install -r test/py/requirements.txt - - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/${TEST_PY_BD}; - export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; + - export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}"
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fi
The repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fiThe repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Regards, Simon
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fiThe repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Hi Tom,
On Sun, 15 Mar 2020 at 07:03, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fiThe repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Yes I see. But in this case we are only building a single board so there should be no difference.
BTW it looks like we are not using the -l flag for the 'multiple build' case, so we shouldn't see the (BOARD1,BOARD2) thing.
Regards, Simon
On Sun, Mar 15, 2020 at 09:07:54AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 07:03, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fiThe repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Yes I see. But in this case we are only building a single board so there should be no difference.
BTW it looks like we are not using the -l flag for the 'multiple build' case, so we shouldn't see the (BOARD1,BOARD2) thing.
Perhaps this is a good example of why I'm asking for the commit message to be clearer about the tests being changed :) But isn't this also the same build area as the general builds?
Hi Tom,
On Sun, 15 Mar 2020 at 09:23, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 15, 2020 at 09:07:54AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 07:03, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote:
Avoid needing to know about the internal .bm-work directory, by passing the -w flag to buildman.
Also drop the repeated call to buildman since the first one should show all the expected output. We only need to use -s if we are building multiple boards and want the errors to be coalesced. In this case we are only building a single board.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b29d59d942..bbd05aa872 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -29,11 +29,11 @@ stages: script: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration.
- export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; if [[ $ret -ne 0 && $ret -ne 129 ]]; then
tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; exit $ret; fiThe repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Yes I see. But in this case we are only building a single board so there should be no difference.
BTW it looks like we are not using the -l flag for the 'multiple build' case, so we shouldn't see the (BOARD1,BOARD2) thing.
Perhaps this is a good example of why I'm asking for the commit message to be clearer about the tests being changed :) But isn't this also the same build area as the general builds?
Yes I'll update the commit subjects.
This is the buildman_and_testpy_template section, which is only used to build a single board, so far as I can tell.
Regards, Simon
On Sun, Mar 15, 2020 at 09:50:30AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 09:23, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 15, 2020 at 09:07:54AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 07:03, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote: > Avoid needing to know about the internal .bm-work directory, by passing > the -w flag to buildman. > > Also drop the repeated call to buildman since the first one should show > all the expected output. We only need to use -s if we are building > multiple boards and want the errors to be coalesced. In this case we are > only building a single board. > > Signed-off-by: Simon Glass sjg@chromium.org > --- > > .gitlab-ci.yml | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index b29d59d942..bbd05aa872 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -29,11 +29,11 @@ stages: > script: > # From buildman, exit code 129 means warnings only. If we've been asked to > # use clang only do one configuration. > + - export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD} > - ret=0; > - tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} > - || ret=$?; > + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E > + --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; > if [[ $ret -ne 0 && $ret -ne 129 ]]; then > - tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; > exit $ret; > fi
The repeated call is so that when we have a CI error from buildman the error is at the bottom of the output and we don't have to hunt for it, so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Yes I see. But in this case we are only building a single board so there should be no difference.
BTW it looks like we are not using the -l flag for the 'multiple build' case, so we shouldn't see the (BOARD1,BOARD2) thing.
Perhaps this is a good example of why I'm asking for the commit message to be clearer about the tests being changed :) But isn't this also the same build area as the general builds?
Yes I'll update the commit subjects.
This is the buildman_and_testpy_template section, which is only used to build a single board, so far as I can tell.
OK then yes, the cases where we are building a single board it would make sense to not do the second build. Thanks!
Hi Tom,
On Sun, 15 Mar 2020 at 10:18, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 15, 2020 at 09:50:30AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 09:23, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 15, 2020 at 09:07:54AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 07:03, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:07PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:58, Tom Rini trini@konsulko.com wrote: > > On Fri, Mar 06, 2020 at 08:07:26PM -0700, Simon Glass wrote: > > Avoid needing to know about the internal .bm-work directory, by passing > > the -w flag to buildman. > > > > Also drop the repeated call to buildman since the first one should show > > all the expected output. We only need to use -s if we are building > > multiple boards and want the errors to be coalesced. In this case we are > > only building a single board. > > > > Signed-off-by: Simon Glass sjg@chromium.org > > --- > > > > .gitlab-ci.yml | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index b29d59d942..bbd05aa872 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -29,11 +29,11 @@ stages: > > script: > > # From buildman, exit code 129 means warnings only. If we've been asked to > > # use clang only do one configuration. > > + - export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD} > > - ret=0; > > - tools/buildman/buildman -o /tmp -P -E --board ${TEST_PY_BD} ${OVERRIDE} > > - || ret=$?; > > + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E > > + --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; > > if [[ $ret -ne 0 && $ret -ne 129 ]]; then > > - tools/buildman/buildman -o /tmp -seP --board ${TEST_PY_BD}; > > exit $ret; > > fi > > The repeated call is so that when we have a CI error from buildman the > error is at the bottom of the output and we don't have to hunt for it, > so I'm not sure this is a developer-friendly change.
I don't quite get this, since the two buildman calls are one after the other. What difference do you see in the output?
Instead of having the errors be throughout the page we see something like: arch: +BOARD1 BOARD2 +(BOARD1,BOARD2) error..
At the bottom of the page. So you open the failed build link, hit "End" and there's where to find what to fix.
Yes I see. But in this case we are only building a single board so there should be no difference.
BTW it looks like we are not using the -l flag for the 'multiple build' case, so we shouldn't see the (BOARD1,BOARD2) thing.
Perhaps this is a good example of why I'm asking for the commit message to be clearer about the tests being changed :) But isn't this also the same build area as the general builds?
Yes I'll update the commit subjects.
This is the buildman_and_testpy_template section, which is only used to build a single board, so far as I can tell.
OK then yes, the cases where we are building a single board it would make sense to not do the second build. Thanks!
OK.
Also I think I got my 'kea' gitlab runner working. Do you want to try to add it to the group runners?
Regards, Simon
Bash allows for variables to expand only if non-empty:
$ var=test $ echo ${var:+"$var"} test $ echo ${var:+"-k $var"} -k test $ var= $ echo ${var:+"-k $var"}
Use this feature to avoid the workaround.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bbd05aa872..05f56c6d19 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -36,17 +36,13 @@ stages: if [[ $ret -ne 0 && $ret -ne 129 ]]; then exit $ret; fi - # "not a_test_which_does_not_exist" is a dummy -k parameter which will - # never prevent any test from running. That way, we can always pass - # "-k something" even when $TEST_PY_TEST_SPEC doesnt need a custom - # value. - virtualenv -p /usr/bin/python3 /tmp/venv - . /tmp/venv/bin/activate - pip install -r test/py/requirements.txt - export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} - -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}" + ${TEST_PY_TEST_SPEC:+"-k ${TEST_PY_TEST_SPEC}"} --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; ret=$?; if [[ $ret -ne 0 ]]; then
On Fri, Mar 06, 2020 at 08:07:27PM -0700, Simon Glass wrote:
Bash allows for variables to expand only if non-empty:
$ var=test $ echo ${var:+"$var"} test $ echo ${var:+"-k $var"} -k test $ var= $ echo ${var:+"-k $var"}
Use this feature to avoid the workaround.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bbd05aa872..05f56c6d19 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -36,17 +36,13 @@ stages: if [[ $ret -ne 0 && $ret -ne 129 ]]; then exit $ret; fi
- # "not a_test_which_does_not_exist" is a dummy -k parameter which will
- # never prevent any test from running. That way, we can always pass
- # "-k something" even when $TEST_PY_TEST_SPEC doesnt need a custom
- # value.
- virtualenv -p /usr/bin/python3 /tmp/venv
- . /tmp/venv/bin/activate
- pip install -r test/py/requirements.txt
- export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID}
-k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}"
${TEST_PY_TEST_SPEC:+"-k ${TEST_PY_TEST_SPEC}"} --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; ret=$?; if [[ $ret -ne 0 ]]; then
Please change the comment to note that other less-than-obvious bash feature being used here, thanks!
Sometimes we don't want to get an error with warnings. Add a -W option to support this. If buildman detects warnings (and no error) it will return an exit code of 0 (success).
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/README | 2 +- tools/buildman/cmdline.py | 2 ++ tools/buildman/control.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/buildman/README b/tools/buildman/README index abbbbea9f2..87bd8ee9d7 100644 --- a/tools/buildman/README +++ b/tools/buildman/README @@ -1079,7 +1079,7 @@ When doing builds, Buildman's return code will reflect the overall result:
0 (success) No errors or warnings found 128 Errors found - 129 Warnings found + 129 Warnings found (use -W to return 0 in this case)
How to change from MAKEALL diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index 74b410010d..f387aeb1cf 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -108,6 +108,8 @@ def ParseArgs(): default=False, help='Run make with V=1, logging all output') parser.add_option('-w', '--work-in-output', action='store_true', default=False, help='Use the output directory as the work directory') + parser.add_option('-W', '--ignore-warnings', action='store_true', + default=False, help='Return success even if there are warnings') parser.add_option('-x', '--exclude', dest='exclude', type='string', action='append', help='Specify a list of boards to exclude, separated by comma') diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 5d80400f7a..ded4360250 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -386,6 +386,6 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, options.keep_outputs, options.verbose) if fail: return 128 - elif warned: + elif warned and not options.ignore_warnings: return 129 return 0
It doesn't seem to make sense to tell buildman to report warning as errors (thus ensuring there are no warnings) and then ignore the warnings.
We can simplify the logic now that we can tell buildman to ignore warnings.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 05f56c6d19..9de6592d1a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -30,12 +30,8 @@ stages: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration. - export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD} - - ret=0; - tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E - --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - exit $ret; - fi + - tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -W + --board ${TEST_PY_BD} ${OVERRIDE} - virtualenv -p /usr/bin/python3 /tmp/venv - . /tmp/venv/bin/activate - pip install -r test/py/requirements.txt @@ -54,8 +50,8 @@ build all 32bit ARM platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E arm -x aarch64 || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -PW arm -x aarch64 || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -68,8 +64,8 @@ build all 64bit ARM platforms: - . /tmp/venv/bin/activate - pip install pyelftools - ret=0; - ./tools/buildman/buildman -o /tmp -P -E aarch64 || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -PW aarch64 || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -79,8 +75,8 @@ build all PowerPC platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E powerpc || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -PW powerpc || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -90,8 +86,8 @@ build all other platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E -x arm,powerpc || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -PW -x arm,powerpc || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi;
On Fri, Mar 06, 2020 at 08:07:29PM -0700, Simon Glass wrote:
It doesn't seem to make sense to tell buildman to report warning as errors (thus ensuring there are no warnings) and then ignore the warnings.
We can simplify the logic now that we can tell buildman to ignore warnings.
Signed-off-by: Simon Glass sjg@chromium.org
.gitlab-ci.yml | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 05f56c6d19..9de6592d1a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -30,12 +30,8 @@ stages: # From buildman, exit code 129 means warnings only. If we've been asked to # use clang only do one configuration. - export UBOOT_TRAVIS_BUILD_DIR=/tmp/${TEST_PY_BD}
- ret=0;
tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E--board ${TEST_PY_BD} ${OVERRIDE} || ret=$?;if [[ $ret -ne 0 && $ret -ne 129 ]]; thenexit $ret;fi
- tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -W
--board ${TEST_PY_BD} ${OVERRIDE}
- virtualenv -p /usr/bin/python3 /tmp/venv
- . /tmp/venv/bin/activate
- pip install -r test/py/requirements.txt
Maybe this whole bit of logic needs some re-thinking. We pass -E to get warnings-as-errors so I think it's better to drop this new flag, fix the test to just check for non-zero and a Fixes tag for 329f5ef51d2e8de79fe2e846f2c2905da9530a71 as that should have done this in the first place I think. Thanks!
Ensure that this SPL test runs on gitlab.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9de6592d1a..26c8fb3199 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -182,7 +182,7 @@ sandbox_spl test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_spl" - TEST_PY_TEST_SPEC: "test_ofplatdata" + TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff" <<: *buildman_and_testpy_dfn
evb-ast2500 test.py:
At present buildman removes any directory it doesn't intend to write output into. This is overly expansive since if the output directory happens to be somewhere with existing files, they may be removed. Using an existing directory for buildman is not a good practice, but since the result might be catastrophic, it is best to guard against it.
A previous commit[1] fixed this by refusing to write to a subdirectory of the current directory, assumed to have U-Boot source code. But we can do better by only removing directories that look like the ones buildman creates.
Update the code to do this and add a test.
Signed-off-by: Simon Glass sjg@chromium.org
[1] 409fc029c40 tools: buildman: Don't use the working dir as build dir
---
tools/buildman/builder.py | 27 ++++++++++++++++++++++----- tools/buildman/test.py | 20 ++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index a41d0b316e..30ec4254f8 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -485,6 +485,7 @@ class Builder: if self.commits: commit = self.commits[commit_upto] subject = commit.subject.translate(trans_valid_chars) + # See _GetOutputSpaceRemovals() which parses this name commit_dir = ('%02d_of_%02d_g%s_%s' % (commit_upto + 1, self.commit_count, commit.hash, subject[:20])) elif not self.no_subdirs: @@ -1525,12 +1526,15 @@ class Builder: for thread in range(max_threads): self._PrepareThread(thread, setup_git)
- def _PrepareOutputSpace(self): + def _GetOutputSpaceRemovals(self): """Get the output directories ready to receive files.
- We delete any output directories which look like ones we need to - create. Having left over directories is confusing when the user wants - to check the output manually. + Figure out what needs to be deleted in the output directory before it + can be used. We only delete old buildman directories which have the + expected name pattern. See _GetOutputDir(). + + Returns: + List of full paths of directories to remove """ if not self.commits: return @@ -1541,7 +1545,20 @@ class Builder: to_remove = [] for dirname in glob.glob(os.path.join(self.base_dir, '*')): if dirname not in dir_list: - to_remove.append(dirname) + leaf = dirname[len(self.base_dir) + 1:] + m = re.match('[0-9]+_of_[0-9]+_g[0-9a-f]+_.*', leaf) + if m: + to_remove.append(dirname) + return to_remove + + def _PrepareOutputSpace(self): + """Get the output directories ready to receive files. + + We delete any output directories which look like ones we need to + create. Having left over directories is confusing when the user wants + to check the output manually. + """ + to_remove = self._GetOutputSpaceRemovals() if to_remove: Print('Removing %d old build directories' % len(to_remove), newline=False) diff --git a/tools/buildman/test.py b/tools/buildman/test.py index acd862b3b0..2aaedf44ac 100644 --- a/tools/buildman/test.py +++ b/tools/buildman/test.py @@ -22,6 +22,7 @@ import commit import terminal import test_util import toolchain +import tools
use_network = True
@@ -469,6 +470,25 @@ class TestBuild(unittest.TestCase): self.assertEqual('HOSTCC=clang CC=clang', tc.GetEnvArgs(toolchain.VAR_MAKE_ARGS))
+ def testPrepareOutputSpace(self): + def _Touch(fname): + tools.WriteFile(os.path.join(base_dir, fname), b'') + + base_dir = tempfile.mkdtemp() + + # Add various files that we want removed and left alone + to_remove = ['01_of_22_g0982734987_title', '102_of_222_g92bf_title', + '01_of_22_g2938abd8_title'] + to_leave = ['something_else', '01-something.patch', '01_of_22_another'] + for name in to_remove + to_leave: + _Touch(name) + + build = builder.Builder(self.toolchains, base_dir, None, 1, 2) + build.commits = self.commits + build.commit_count = len(commits) + result = set(build._GetOutputSpaceRemovals()) + expected = set([os.path.join(base_dir, f) for f in to_remove]) + self.assertEqual(expected, result)
if __name__ == "__main__": unittest.main()
This is useful in some situations, in particular with -w and when building in-tree. Now that we are more careful about what we remove in _PrepareOutputSpace(), it should be save to relax this restriction.
Update the progress information also so it is clear what buildman is doing. Remove files can take a long time.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/builder.py | 3 ++- tools/buildman/control.py | 23 ----------------------- tools/buildman/func_test.py | 9 --------- 3 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 30ec4254f8..70c55c588a 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -1560,10 +1560,11 @@ class Builder: """ to_remove = self._GetOutputSpaceRemovals() if to_remove: - Print('Removing %d old build directories' % len(to_remove), + Print('Removing %d old build directories...' % len(to_remove), newline=False) for dirname in to_remove: shutil.rmtree(dirname) + Print('done')
def BuildBoards(self, commits, board_selected, keep_outputs, verbose): """Build all commits for a list of boards diff --git a/tools/buildman/control.py b/tools/buildman/control.py index ded4360250..7d31863c63 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -85,28 +85,6 @@ def ShowActions(series, why_selected, boards_selected, builder, options, for warning in board_warnings: print(col.Color(col.YELLOW, warning))
-def CheckOutputDir(output_dir): - """Make sure that the output directory is not within the current directory - - If we try to use an output directory which is within the current directory - (which is assumed to hold the U-Boot source) we may end up deleting the - U-Boot source code. Detect this and print an error in this case. - - Args: - output_dir: Output directory path to check - """ - path = os.path.realpath(output_dir) - cwd_path = os.path.realpath('.') - while True: - if os.path.realpath(path) == cwd_path: - Print("Cannot use output directory '%s' since it is within the current directory '%s'" % - (path, cwd_path)) - sys.exit(1) - parent = os.path.dirname(path) - if parent == path: - break - path = parent - def ShowToolchainInfo(boards, toolchains, print_arch, print_prefix): """Show information about a the tool chain used by one or more boards
@@ -331,7 +309,6 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, output_dir = os.path.join(options.output_dir, dirname) if clean_dir and os.path.exists(output_dir): shutil.rmtree(output_dir) - CheckOutputDir(output_dir) builder = Builder(toolchains, output_dir, options.git_dir, options.threads, options.jobs, gnu_make=gnu_make, checkout=True, show_unknown=options.show_unknown, step=options.step, diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index f9f8f80593..2a256a9263 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -534,15 +534,6 @@ class TestFunctional(unittest.TestCase): self.assertEqual(self._builder.count, self._total_builds) self.assertEqual(self._builder.fail, 0)
- def testBadOutputDir(self): - """Test building with an output dir the same as out current dir""" - self._test_branch = '/__dev/__testbranch' - with self.assertRaises(SystemExit): - self._RunControl('-b', self._test_branch, '-o', os.getcwd()) - with self.assertRaises(SystemExit): - self._RunControl('-b', self._test_branch, '-o', - os.path.join(os.getcwd(), 'test')) - def testWorkInOutput(self): """Test the -w option which should write directly to the output dir""" board_list = board.Boards()
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/py/conftest.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/test/py/conftest.py b/test/py/conftest.py index 34ac4fb062..8079cd7305 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -141,17 +141,13 @@ def pytest_configure(config):
if config.getoption('build'): if build_dir != source_dir: - o_opt = 'O=%s' % build_dir + dest_args = ['-o', build_dir, '-w'] else: - o_opt = '' - cmds = ( - ['make', o_opt, '-s', board_type + '_defconfig'], - ['make', o_opt, '-s', '-j8'], - ) - with log.section('make'): - runner = log.get_runner('make', sys.stdout) - for cmd in cmds: - runner.run(cmd, cwd=source_dir) + dest_args = ['-i'] + cmd = ['buildman', '--board', board_type] + dest_args + with log.section('buildman'): + runner = log.get_runner('buildman', sys.stdout) + runner.run(cmd, cwd=source_dir) runner.close() log.status_pass('OK')
On 3/6/20 8:07 PM, Simon Glass wrote:
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
I far prefer using make here; this requires zero setup of buildman (e.g. the config file and specific toolchains), and so it much *less* of a pain.
On Mon, Mar 09, 2020 at 11:10:55AM -0600, Stephen Warren wrote:
On 3/6/20 8:07 PM, Simon Glass wrote:
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
I far prefer using make here; this requires zero setup of buildman (e.g. the config file and specific toolchains), and so it much *less* of a pain.
I have to agree here. Keeping our test suite as dependency-free as possible is important. But... that's also not what's going on in the code. We don't set ARCH from what I can see, and of course don't use it. We don't set the CROSS_COMPILER from the snippet in question, only the output directory. Today, looking at the Travis/GitLab CI scripts we don't even build via test.py but rather buildman prior to calling test.py. And I don't think I saw that changing in this series either.
Hi Tom,
On Mon, 9 Mar 2020 at 11:42, Tom Rini trini@konsulko.com wrote:
On Mon, Mar 09, 2020 at 11:10:55AM -0600, Stephen Warren wrote:
On 3/6/20 8:07 PM, Simon Glass wrote:
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
I far prefer using make here; this requires zero setup of buildman (e.g. the config file and specific toolchains), and so it much *less* of a pain.
I have to agree here. Keeping our test suite as dependency-free as possible is important. But... that's also not what's going on in the code. We don't set ARCH from what I can see, and of course don't use it. We don't set the CROSS_COMPILER from the snippet in question, only the output directory. Today, looking at the Travis/GitLab CI scripts we don't even build via test.py but rather buildman prior to calling test.py. And I don't think I saw that changing in this series either.
I mean that to run pytest I have to do:
PATH=$PATH:tools/buildman ARCH=`buildman -a zynq_zybo` CROSS_COMPILE=`buildman -A zynq_zybo` \ test/py/test.py -B zynq_zybo --id sjg-zynq_zybo --build-dir ../current/zynq_zybo --build
which is a bit of a pain.
With this change I can do:
test/py/test.py -B zynq_zybo --id sjg-zynq_zybo --build-dir ../current/zynq_zybo --build
Regards, Simon
On Tue, Mar 10, 2020 at 08:27:47PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:42, Tom Rini trini@konsulko.com wrote:
On Mon, Mar 09, 2020 at 11:10:55AM -0600, Stephen Warren wrote:
On 3/6/20 8:07 PM, Simon Glass wrote:
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
I far prefer using make here; this requires zero setup of buildman (e.g. the config file and specific toolchains), and so it much *less* of a pain.
I have to agree here. Keeping our test suite as dependency-free as possible is important. But... that's also not what's going on in the code. We don't set ARCH from what I can see, and of course don't use it. We don't set the CROSS_COMPILER from the snippet in question, only the output directory. Today, looking at the Travis/GitLab CI scripts we don't even build via test.py but rather buildman prior to calling test.py. And I don't think I saw that changing in this series either.
I mean that to run pytest I have to do:
PATH=$PATH:tools/buildman ARCH=`buildman -a zynq_zybo` CROSS_COMPILE=`buildman -A zynq_zybo` \ test/py/test.py -B zynq_zybo --id sjg-zynq_zybo --build-dir ../current/zynq_zybo --build
which is a bit of a pain.
With this change I can do:
test/py/test.py -B zynq_zybo --id sjg-zynq_zybo --build-dir ../current/zynq_zybo --build
Right. The commit message isn't clear as the CI loops build the board with buildman first. Second, we don't use ARCH= when building U-Boot, so we could just drop that from buildman I suspect. Third, no, I think it's important to NOT require buildman to be builder here and setting CROSS_COMPILE in the environment is fine and makes integration with other systems easier. Thanks!
Hi Stephen,
On Mon, 9 Mar 2020 at 11:10, Stephen Warren swarren@wwwdotorg.org wrote:
On 3/6/20 8:07 PM, Simon Glass wrote:
It is a pain to have to set the ARCH and CROSS_COMPILE environment variables when using test.py's --build option. It is possible to get these using the -A and -a options from buildman. But it seems better to just use buildman to do the build.
Remove the manual 'make' logic in test/py and use buildman instead.
I far prefer using make here; this requires zero setup of buildman (e.g. the config file and specific toolchains), and so it much *less* of a pain.
For people who don't have an existing setup though, it is tricky. And having to set those environment variables is a hassle.
'buildman --fetch-arch all' will download and set up toolchains.
Perhaps we could make this optional?
Regards, Simon
It seems unnecessary to read the exit code and then check it again. Drop this and just let the test.py provide the exit code directly.
Signed-off-by: Simon Glass sjg@chromium.org ---
.gitlab-ci.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 26c8fb3199..98adfa7d80 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -39,11 +39,7 @@ stages: export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} ${TEST_PY_TEST_SPEC:+"-k ${TEST_PY_TEST_SPEC}"} - --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; - ret=$?; - if [[ $ret -ne 0 ]]; then - exit $ret; - fi + --build-dir "$UBOOT_TRAVIS_BUILD_DIR"
build all 32bit ARM platforms: tags: [ 'all' ]
On Fri, Mar 06, 2020 at 08:07:14PM -0700, Simon Glass wrote:
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script:
- Option to run a single build in a specified output directory
- Allow ignoring warnings
- Removes a restriction on the build output directory
It also
- moves test.py over to use buildman for the --build option
- makes one change to azure since the same approach should be possible there
- fixes a few minor problems noticed in main and sandbox docs
One general comment is that while it's clear from this series that you're focusing on test.py invocation most of the time, a lot of the commit messages aren't clear that you're changing the buildman_and_testpy_template stanza.
Also, while you're keeping Azure in sync (as it's largely based on the GitLab logic), Travis is being left out. But the GitLab logic is based on the Travis logic and as much as I dislike Travis for being slow and having network induced failures, it's still a valid and supported CI flow that we can't drop, so please keep it in sync with these improvements, thanks!
Hi Tom,
On Mon, 9 Mar 2020 at 11:55, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:14PM -0700, Simon Glass wrote:
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script:
- Option to run a single build in a specified output directory
- Allow ignoring warnings
- Removes a restriction on the build output directory
It also
- moves test.py over to use buildman for the --build option
- makes one change to azure since the same approach should be possible there
- fixes a few minor problems noticed in main and sandbox docs
One general comment is that while it's clear from this series that you're focusing on test.py invocation most of the time, a lot of the commit messages aren't clear that you're changing the buildman_and_testpy_template stanza.
I'm not sure what you are looking for there. Do you want the commit message to mention which part of the gitlab file is being changed?
Also, while you're keeping Azure in sync (as it's largely based on the GitLab logic), Travis is being left out. But the GitLab logic is based on the Travis logic and as much as I dislike Travis for being slow and having network induced failures, it's still a valid and supported CI flow that we can't drop, so please keep it in sync with these improvements, thanks!
OK.
Regards, Simon
On Sat, Mar 14, 2020 at 09:10:09PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:55, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:14PM -0700, Simon Glass wrote:
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script:
- Option to run a single build in a specified output directory
- Allow ignoring warnings
- Removes a restriction on the build output directory
It also
- moves test.py over to use buildman for the --build option
- makes one change to azure since the same approach should be possible there
- fixes a few minor problems noticed in main and sandbox docs
One general comment is that while it's clear from this series that you're focusing on test.py invocation most of the time, a lot of the commit messages aren't clear that you're changing the buildman_and_testpy_template stanza.
I'm not sure what you are looking for there. Do you want the commit message to mention which part of the gitlab file is being changed?
I mean a lot of places say something like "Change gitlab ..." but aren't changing any of the world builds, only test.py stuff, so I would prefer "Change gitlab when using test.py ..." Thanks!
Hi Tom,
On Sun, 15 Mar 2020 at 07:02, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:09PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:55, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:14PM -0700, Simon Glass wrote:
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script:
- Option to run a single build in a specified output directory
- Allow ignoring warnings
- Removes a restriction on the build output directory
It also
- moves test.py over to use buildman for the --build option
- makes one change to azure since the same approach should be possible there
- fixes a few minor problems noticed in main and sandbox docs
One general comment is that while it's clear from this series that you're focusing on test.py invocation most of the time, a lot of the commit messages aren't clear that you're changing the buildman_and_testpy_template stanza.
I'm not sure what you are looking for there. Do you want the commit message to mention which part of the gitlab file is being changed?
I mean a lot of places say something like "Change gitlab ..." but aren't changing any of the world builds, only test.py stuff, so I would prefer "Change gitlab when using test.py ..."
OK I will try. I've got the changes to all three environments in a single commit at present, so we don't end up with double the commis. Or would you prefer it split out?
One more thing...I notice that gitlab and azure use 'set -ex' to avoid needing to check errors in the script. Is is possible for travis to do that do, or is there some restriction?
Thanks!
-- Tom
Regards, Simon
On Sun, Mar 15, 2020 at 09:07:48AM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 15 Mar 2020 at 07:02, Tom Rini trini@konsulko.com wrote:
On Sat, Mar 14, 2020 at 09:10:09PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Mar 2020 at 11:55, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 06, 2020 at 08:07:14PM -0700, Simon Glass wrote:
At present there are several things in the gitlab script which work around limitations in buildman. With a few small feature additions these can be removed.
This series adds some new features to buildman and simplifies the script:
- Option to run a single build in a specified output directory
- Allow ignoring warnings
- Removes a restriction on the build output directory
It also
- moves test.py over to use buildman for the --build option
- makes one change to azure since the same approach should be possible there
- fixes a few minor problems noticed in main and sandbox docs
One general comment is that while it's clear from this series that you're focusing on test.py invocation most of the time, a lot of the commit messages aren't clear that you're changing the buildman_and_testpy_template stanza.
I'm not sure what you are looking for there. Do you want the commit message to mention which part of the gitlab file is being changed?
I mean a lot of places say something like "Change gitlab ..." but aren't changing any of the world builds, only test.py stuff, so I would prefer "Change gitlab when using test.py ..."
OK I will try. I've got the changes to all three environments in a single commit at present, so we don't end up with double the commis. Or would you prefer it split out?
Doing all CIs in step makes sense. So something like...: Azure/GitLab/Travis: Change test.py and buildman ...
One more thing...I notice that gitlab and azure use 'set -ex' to avoid needing to check errors in the script. Is is possible for travis to do that do, or is there some restriction?
Not sure. When in doubt I always hack the heck out of .travis.yml to just be a testcase of what I want to figure out, push and see. Thanks!
participants (3)
-
Simon Glass -
Stephen Warren -
Tom Rini