| Message ID | 20200315234303.18598-16-sjg@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | gitlab: Simplify the test script | expand |
On Sun, Mar 15, 2020 at 05:42:59PM -0600, 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. > > The simplist thing is to just drop the -E flag. This allows us to drop the > check for exit code 129. > > Dropping -E is not enough to cover all warnings though. For example this > warning: > > ===================== WARNING ====================== > This board does not use CONFIG_DM. CONFIG_DM will be > compulsory starting with the v2020.01 release. > Failure to update may result in board removal. > See doc/driver-model/migration.rst for more info. > > also causes buildman to return an exit code of 129. So use -W to suppress > that, since otherwise the build will fail. > > Signed-off-by: Simon Glass <sjg at chromium.org> > Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) Ah, we have something funny going on, or at least not clear enough. We need and want -E here as that causes us to build with -Werror and so warnings become errors and the build fails. We still ignore warnings such as "go convert X to DM" (which is its own issue to deal with) and also dtc warnings (which is its own issue to deal with).
Hi Tom, On Mon, 16 Mar 2020 at 15:21, Tom Rini <trini at konsulko.com> wrote: > > On Sun, Mar 15, 2020 at 05:42:59PM -0600, 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. > > > > The simplist thing is to just drop the -E flag. This allows us to drop the > > check for exit code 129. > > > > Dropping -E is not enough to cover all warnings though. For example this > > warning: > > > > ===================== WARNING ====================== > > This board does not use CONFIG_DM. CONFIG_DM will be > > compulsory starting with the v2020.01 release. > > Failure to update may result in board removal. > > See doc/driver-model/migration.rst for more info. > > > > also causes buildman to return an exit code of 129. So use -W to suppress > > that, since otherwise the build will fail. > > > > Signed-off-by: Simon Glass <sjg at chromium.org> > > Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) > > Ah, we have something funny going on, or at least not clear enough. We > need and want -E here as that causes us to build with -Werror and so > warnings become errors and the build fails. We still ignore warnings > such as "go convert X to DM" (which is its own issue to deal with) and > also dtc warnings (which is its own issue to deal with). OK I see. So should we add both -E and -W? Perhaps the is the behaviour we want? Regards, Simon
On Tue, Mar 17, 2020 at 10:11:52AM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 16 Mar 2020 at 15:21, Tom Rini <trini at konsulko.com> wrote: > > > > On Sun, Mar 15, 2020 at 05:42:59PM -0600, 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. > > > > > > The simplist thing is to just drop the -E flag. This allows us to drop the > > > check for exit code 129. > > > > > > Dropping -E is not enough to cover all warnings though. For example this > > > warning: > > > > > > ===================== WARNING ====================== > > > This board does not use CONFIG_DM. CONFIG_DM will be > > > compulsory starting with the v2020.01 release. > > > Failure to update may result in board removal. > > > See doc/driver-model/migration.rst for more info. > > > > > > also causes buildman to return an exit code of 129. So use -W to suppress > > > that, since otherwise the build will fail. > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org> > > > Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) > > > > Ah, we have something funny going on, or at least not clear enough. We > > need and want -E here as that causes us to build with -Werror and so > > warnings become errors and the build fails. We still ignore warnings > > such as "go convert X to DM" (which is its own issue to deal with) and > > also dtc warnings (which is its own issue to deal with). > > OK I see. So should we add both -E and -W? Perhaps the is the behaviour we want? I'm not sure. We need -E, yes. I guess I'm ambivalent on if we should make buildman have an option to return 0 still here, or just have CI know that specific error-code is OK as we've otherwise turned code warnings to errors. Having typed that, I see the problem I have now. We need to make it clearer, in both the -W and -E cases what kinds of warnings we're talking about. -E enables -Werror to the C compiler. -W suppresses the kinds of WARNINGS we echo to console but I'm not sure how to best characterize.
Hi Tom, On Tue, 17 Mar 2020 at 10:22, Tom Rini <trini at konsulko.com> wrote: > > On Tue, Mar 17, 2020 at 10:11:52AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Mon, 16 Mar 2020 at 15:21, Tom Rini <trini at konsulko.com> wrote: > > > > > > On Sun, Mar 15, 2020 at 05:42:59PM -0600, 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. > > > > > > > > The simplist thing is to just drop the -E flag. This allows us to drop the > > > > check for exit code 129. > > > > > > > > Dropping -E is not enough to cover all warnings though. For example this > > > > warning: > > > > > > > > ===================== WARNING ====================== > > > > This board does not use CONFIG_DM. CONFIG_DM will be > > > > compulsory starting with the v2020.01 release. > > > > Failure to update may result in board removal. > > > > See doc/driver-model/migration.rst for more info. > > > > > > > > also causes buildman to return an exit code of 129. So use -W to suppress > > > > that, since otherwise the build will fail. > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org> > > > > Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) > > > > > > Ah, we have something funny going on, or at least not clear enough. We > > > need and want -E here as that causes us to build with -Werror and so > > > warnings become errors and the build fails. We still ignore warnings > > > such as "go convert X to DM" (which is its own issue to deal with) and > > > also dtc warnings (which is its own issue to deal with). > > > > OK I see. So should we add both -E and -W? Perhaps the is the behaviour we want? > > I'm not sure. We need -E, yes. I guess I'm ambivalent on if we should > make buildman have an option to return 0 still here, or just have CI > know that specific error-code is OK as we've otherwise turned code > warnings to errors. > > Having typed that, I see the problem I have now. We need to make it > clearer, in both the -W and -E cases what kinds of warnings we're > talking about. -E enables -Werror to the C compiler. -W suppresses the > kinds of WARNINGS we echo to console but I'm not sure how to best > characterize. OK shall I beef up the help text in buildman for those two? Regards, Simon
On Tue, Mar 17, 2020 at 10:32:32AM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 17 Mar 2020 at 10:22, Tom Rini <trini at konsulko.com> wrote: > > > > On Tue, Mar 17, 2020 at 10:11:52AM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Mon, 16 Mar 2020 at 15:21, Tom Rini <trini at konsulko.com> wrote: > > > > > > > > On Sun, Mar 15, 2020 at 05:42:59PM -0600, 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. > > > > > > > > > > The simplist thing is to just drop the -E flag. This allows us to drop the > > > > > check for exit code 129. > > > > > > > > > > Dropping -E is not enough to cover all warnings though. For example this > > > > > warning: > > > > > > > > > > ===================== WARNING ====================== > > > > > This board does not use CONFIG_DM. CONFIG_DM will be > > > > > compulsory starting with the v2020.01 release. > > > > > Failure to update may result in board removal. > > > > > See doc/driver-model/migration.rst for more info. > > > > > > > > > > also causes buildman to return an exit code of 129. So use -W to suppress > > > > > that, since otherwise the build will fail. > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org> > > > > > Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) > > > > > > > > Ah, we have something funny going on, or at least not clear enough. We > > > > need and want -E here as that causes us to build with -Werror and so > > > > warnings become errors and the build fails. We still ignore warnings > > > > such as "go convert X to DM" (which is its own issue to deal with) and > > > > also dtc warnings (which is its own issue to deal with). > > > > > > OK I see. So should we add both -E and -W? Perhaps the is the behaviour we want? > > > > I'm not sure. We need -E, yes. I guess I'm ambivalent on if we should > > make buildman have an option to return 0 still here, or just have CI > > know that specific error-code is OK as we've otherwise turned code > > warnings to errors. > > > > Having typed that, I see the problem I have now. We need to make it > > clearer, in both the -W and -E cases what kinds of warnings we're > > talking about. -E enables -Werror to the C compiler. -W suppresses the > > kinds of WARNINGS we echo to console but I'm not sure how to best > > characterize. > > OK shall I beef up the help text in buildman for those two? And the commit message for adding -W, yes, thanks!
diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index 50d00fa899..35c1c8e0d4 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -247,8 +247,8 @@ jobs: cd ${WORK_DIR} 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 + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -W --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; + if [[ $ret -ne 0 ]]; then tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -se --board ${TEST_PY_BD}; exit $ret; fi @@ -396,8 +396,8 @@ jobs: cat << "EOF" >> build.sh if [[ "${BUILDMAN}" != "" ]]; then ret=0; - tools/buildman/buildman -o /tmp -P -E ${BUILDMAN} ${OVERRIDE} || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + tools/buildman/buildman -o /tmp -P -W ${BUILDMAN} ${OVERRIDE} || ret=$?; + if [[ $ret -ne 0 ]]; then tools/buildman/buildman -o /tmp -seP ${BUILDMAN}; exit $ret; fi; diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 48b90b2ba3..db68a5e7a8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -27,13 +27,12 @@ stages: after_script: - rm -rf /tmp/uboot-test-hooks /tmp/venv script: - # From buildman, exit code 129 means warnings only. If we've been asked to - # use clang only do one configuration. + # 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 + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -W --board ${TEST_PY_BD} ${OVERRIDE} || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + if [[ $ret -ne 0 ]]; then tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -se --board ${TEST_PY_BD}; exit $ret; @@ -57,8 +56,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 -P -W arm -x aarch64 || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -71,8 +70,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 -P -W aarch64 || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -82,8 +81,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 -P -W powerpc || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; @@ -93,8 +92,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 -P -W -x arm,powerpc || ret=$?; + if [[ $ret -ne 0 ]]; then ./tools/buildman/buildman -o /tmp -seP; exit $ret; fi; diff --git a/.travis.yml b/.travis.yml index 887654ca96..d5582802a6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -116,13 +116,12 @@ script: # Comments must be outside the command strings below, or the Travis parser # will get confused. # - # From buildman, exit code 129 means warnings only. If we've been asked to - # use clang only do one configuration. + # If we've been asked to use clang only do one configuration. + # # Build a selection of boards if TEST_PY_BD is empty - if [[ "${BUILDMAN}" != "" ]]; then - ret=0; - tools/buildman/buildman -P -E ${BUILDMAN} ${OVERRIDE}|| ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + tools/buildman/buildman -P -W ${BUILDMAN} ${OVERRIDE}; + if [[ $ret -ne 0 ]]; then tools/buildman/buildman -seP ${BUILDMAN}; exit $ret; fi; @@ -136,9 +135,9 @@ script: cp ~/grub2-arm/usr/lib/grub2/arm-efi/grub.efi $UBOOT_TRAVIS_BUILD_DIR/grub_arm.efi; cp ~/grub2-arm64/usr/lib/grub2/arm64-efi/grub.efi $UBOOT_TRAVIS_BUILD_DIR/grub_arm64.efi; ret=0; - tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E + tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -W --board ${TEST_PY_BD} ${OVERRIDE}|| ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then + if [[ $ret -ne 0 ]]; then tools/buildman/buildman -se -o ${UBOOT_TRAVIS_BUILD_DIR} -w --board ${TEST_PY_BD}; exit $ret;
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. The simplist thing is to just drop the -E flag. This allows us to drop the check for exit code 129. Dropping -E is not enough to cover all warnings though. For example this warning: ===================== WARNING ====================== This board does not use CONFIG_DM. CONFIG_DM will be compulsory starting with the v2020.01 release. Failure to update may result in board removal. See doc/driver-model/migration.rst for more info. also causes buildman to return an exit code of 129. So use -W to suppress that, since otherwise the build will fail. Signed-off-by: Simon Glass <sjg at chromium.org> Fixes: 329f5ef51d2 (travis.yml: run buildman with option -E) --- Changes in v2: - Add Fixes tag - Just drop the -E flag - Update travis, azure also .azure-pipelines.yml | 8 ++++---- .gitlab-ci.yml | 23 +++++++++++------------ .travis.yml | 13 ++++++------- 3 files changed, 21 insertions(+), 23 deletions(-)