Rationalize Travis builds #3218

gilles-peskine-arm · 2020-04-21T20:33:37Z

Change what we run on Travis to get better coverage for roughly as much processing power.

The goal of this pull request is to improve the chances that if something breaks on Jenkins, it also breaks on Travis. The advantage of Travis is that its logs are public.

This pull request is about cheap wins. Putting a lot of effort into quantitizing the maximization of coverage is out of scope. Installing new tools in the test environment is out of scope if it's more complicated than declaring a package to install.

We can't run the whole CI on Travis because it would take more processing power than we're willing to use on a free plan and because the full CI uses tools that are not present on Travis. For reference, our current Travis matrix runs two jobs, gcc and clang, which take about 24 min and 13 min respectively on development.

Build times for 10cb160:

basic checks and reference configurations: 6 min
full configuration: 13 min
check compilation guards: 20 min
macOS: 2 min
Windows: 3 min

Coverage improvements:

Run all sanity checks (all.sh check_*)
Test the full configuration, with ASan+UBSan
depends-hashes.pl and depends-pkalgs.pl
There's now a macOS job and a Windows (Visual Studio) job.

Useful Travis documentation:

Build matrix
C compilers
Python
macOS
Windows is still experimental, but a useful addition. Getting Python 3 to work was a mix of trial and error and web searches; https://github.com/cclauss/Travis-CI-Python-on-three-OSes/blob/master/.travis.yml showed me the way.
non-x86 is still experimental and I'm not touching it in this PR.

Backports: 2.16, 2.7.

.travis.yml

Almost everything the selftest program does is in the test suites. But just in case run the selftest program itself once in the full configuration, and once in the default configuration with ASan, in addition to running it out of box. Signed-off-by: Gilles Peskine <[email protected]>

Make it possible to use a compiler that isn't in $PATH, or that's installed with a different name, or even a compiler for a different target such as arm-linux-gnueabi. Signed-off-by: Gilles Peskine <[email protected]>

In practice, we hardly ever get different outcomes, so there is no gain in running tests with different compilers. Experimentally, with the builds and tests we currently do and with the compiler versions on a Travis Ubuntu 16.04, gcc jobs are significantly faster than clang jobs (13 min vs 24 min). So use gcc. Signed-off-by: Gilles Peskine <[email protected]>

Split the build between: * Basic checks * A build in the default configuration with extensive tests * Builds in other configurations with less testing The intent is to have one shorter job with basic tests, and two longer jobs that take roughly the same amount of time (split as evenly as possible while keeping an easy-to-understand separation). Signed-off-by: Gilles Peskine <[email protected]>

Only this job uses doxygen and graphviz. Signed-off-by: Gilles Peskine <[email protected]>

Declare an explicit Python version. Pick 3.5 which is the default version on Ubuntu 16.04. This is necessary on Travis to have a working pip for Python 3. Install Pylint 2.4.4. There's nothing special about this version, it's just the latest version. Signed-off-by: Gilles Peskine <[email protected]>

Different releases have different sets of sanity checks. Keep the list in one place, namely all.sh. Signed-off-by: Gilles Peskine <[email protected]>

This way anything we change in all.sh, such as adding tests for programs/*/*, will be reflected here. The build now uses GCC instead of Clang, which doesn't make much difference in practice. The build now enables ASan and UBSan. The tests now run compat.sh and ssl-opt.sh fully. Signed-off-by: Gilles Peskine <[email protected]>

For the one long job with ASan, use the full configuration. We get more coverage this way, at the cost of a slightly longer runtime which we can afford since the "enumerated configurations" job is slower. Add a default-configuration build to the "basic checks" job. This job is fairly quick (no ASan, no SSL testing). Signed-off-by: Gilles Peskine <[email protected]>

Some jobs don't actually test against GnuTLS, but all.sh checks its presence in all test jobs, so it needs to be installed regardless. Signed-off-by: Gilles Peskine <[email protected]>

Call all.sh to run all the available test_depends_* components. This adds a run of depends-hashes.pl and depends-pkgalgs.pl. Keep invoking test-ref-configs.pl rather than via all.sh so that it doesn't run with ASan. This saves some time and ASan there doesn't turn up much more than in the full config. Signed-off-by: Gilles Peskine <[email protected]>

Add a baremetal build to Travis, to catch inadvertent dependencies on platform functions. The exact choice of target platform doesn't matter for this purpose. Pick one that's present in all.sh, that uses a compiler that's available in the Travis build environment (Ubuntu 16.04), and that happens to be close to the Debian "armel" distribution. Signed-off-by: Gilles Peskine <[email protected]>

Just do the default build with Clang and run the unit tests. The objective is to have one build on a Unix-like platform other than Linux. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

mpg

Looks very good to me!

.travis.yml

gilles-peskine-arm · 2020-04-27T11:43:33Z

The failure in pr-merge was a network glitch. Since the -head job passed and the changes here don't interact with anything that's changed in development since this PR forked, that's good enough CI.

ronald-cron-arm

Only one minor comment.

ronald-cron-arm · 2020-04-28T14:25:25Z

tests/scripts/all.sh

@@ -1572,36 +1576,36 @@ component_test_no_64bit_multiplication () {
 }

 component_build_arm_none_eabi_gcc () {
-    msg "build: arm-none-eabi-gcc, make" # ~ 10s
+    msg "build: ${ARM_GCC_PREFIX}gcc, make" # ~ 10s


Because of this change, change the name of the component? component_build_arm_gcc(_cross_compiler) ?
Same comment for the components below.

I'm not sure, and I'd prefer to defer such a change to when we know how we'll use this feature. If we'll use this feature.

I initially did this because I thought it would be necessary to use a different compiler on Travis to avoid having to install arm-none-eabi-gcc. But it turned out that the compiler we're using on Jenkins is from Ubuntu, so it was straightforward to add this compiler. I left the commit in because even if it isn't necessary now, it could be useful later.

There are two ways this can be used: to substitute a different platform, or to use a compiler installed at a location outside $PATH. For the first use case, we should have different component names. For the second use case, the component should keep its name. My current preference is to declare that the second use case is the one that's primarily intended, and so keep the component names.

Well, I am not really convinced. If ${ARM_GCC_PREFIX} is set up to a value that has nothing to do with arm_none_eabi_gcc then the component has nothing to do with arm_none_eabi_gcc but is called component_build_arm_none_eabi_gcc () which is confusing to me.
Regarding the second use case, making the path to arm_none_eabi_gcc configurable would be enough it seems.
I understand that we are not completely committed to the second use case thus the idea to let it not very well defined yet. But we then go from a situation where things are clear to a situation where things are unclear: the component is named component_build_arm_none_eabi_gcc () but it may not use at all arm_none_eabi_gcc compiler.

How about renaming ARM_GCC_PREFIX to ARM_NONE_EABI_GCC_PREFIX?

Yes, it would be fine by me with this renaming.

This is supposed to be for GCC (or a compiler with a compatible command line interface) targeting arm-none-eabi, so name it accordingly. Signed-off-by: Gilles Peskine <[email protected]>

Otherwise the bignum assembly code is not used. Signed-off-by: Gilles Peskine <[email protected]>

It's pretty fast and adds a little variety. Signed-off-by: Gilles Peskine <[email protected]>

Just show the code size in the logs, for human consumption. Signed-off-by: Gilles Peskine <[email protected]>

The Cortex-A build is similar to Debian armel. The Cortex-M0+ is a handy point of comparison for code size. Put that one last so that it's easy to find in the log. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-04-30T21:34:32Z

Here and in the backports, I added a Cortex-M0+ build, and I made it report the code size. This can be useful information for contributors who might not have an arm cross-compiler handy.

ronald-cron-arm

Thanks for the change on compiler prefix. I have a question about one of the new commits.

ronald-cron-arm · 2020-05-04T07:47:18Z

tests/scripts/all.sh

@@ -1579,7 +1579,7 @@ component_test_no_64bit_multiplication () {
 component_build_arm_none_eabi_gcc () {
    msg "build: ${ARM_NONE_EABI_GCC_PREFIX}gcc, make" # ~ 10s
    scripts/config.py baremetal
-    make CC="${ARM_NONE_EABI_GCC_PREFIX}gcc" AR="${ARM_NONE_EABI_GCC_PREFIX}ar" LD="${ARM_NONE_EABI_GCC_PREFIX}ld" CFLAGS='-Werror -Wall -Wextra' lib
+    make CC="${ARM_NONE_EABI_GCC_PREFIX}gcc" AR="${ARM_NONE_EABI_GCC_PREFIX}ar" LD="${ARM_NONE_EABI_GCC_PREFIX}ld" CFLAGS='-Werror -Wall -Wextra -O1' lib


Just wondering, why not -O2? That's the level of optimization I am used to for release builds.

This is only a build, it won't run, so -O2 has no benefit over -O1, and it's slower. -Oanything_non_zero has the benefit that it activates some compile-time analyses that gcc doesn't try at -O0.

I understand it doesn't run. In my humble Cortex-M experience, -O1 is not really used. -O0 is used for debug builds and then -O2 or -Os are used for "release" builds. Thus if we are at building with some optimization it is maybe a bit better to choose an optimization that is more likely to be used. There are also probably more compile-time analyses when using -O2 compared to -O1. The additional compilation time should be negligible. Thus I see some benefits of choosing -O2 over -O1 here. But if you are not convinced by the above, fine by me, just let me know and I will approve the PR.

The main purpose of this change was actually to build the assembly code that we gate with defined(__OPTIMIZE__). I don't see a need to make the build slower here. That being said, as Manuel said, the arm builds need further rationalizing, but that's beyond the scope of this PR. I've filed #3299. I don't think any further change is warranted in this PR.

ok I wasn't aware of "defined(OPTIMIZE)". Thanks for #3299, this looks very good to me.

mpg

I like the additions in the new commit.

In addition to my comments below, I wanted to note that we now have 3 builds with arm-none-eabi-gcc (disregarding the 2 special ones for NO_UDBL_DIVISION and NO_64BIT_MULTIPLICATION): one for m0plus, one for 5vte, and one for... whatever is the default of the toolchain. That last one may be of questionable use (I couldn't easily find what its value is in the documentation). Maybe having a build for V7-A would be more useful.

However, this PR is "Rationalize Travis builds", not "Rationalize use of arm-none-eabi-gcc", so I'm not requesting any changes here - this can be addressed in another PR, unless you prefer to do it here.

mpg · 2020-05-04T07:45:44Z

.travis.yml

        - tests/scripts/test-ref-configs.pl
+        - tests/scripts/all.sh -k build_arm_none_eabi_gcc_arm5vte build_arm_none_eabi_gcc_m0plus


Note on the commit message: I'm not sure it's quite correct to call the arm5vte build a Cortex-A build. The first cores to be called Cortex-A were implementing version 7 of the architecture, not version 5. https://en.wikipedia.org/wiki/ARM_architecture#Cores

Nevertheless, I agree that v5 was probably closer to what's now A-class than to M-class, so I don't disagree with the reasoning, and I'm not requesting any change, unless if you happen to rework this commit for another reason.

mpg · 2020-05-04T07:54:27Z

tests/scripts/all.sh

+component_build_arm_none_eabi_gcc_m0plus () {
+    msg "build: ${ARM_NONE_EABI_GCC_PREFIX}gcc -mthumb -mcpu=cortex-m0plus" # ~ 10s
+    scripts/config.py baremetal
+    make CC="${ARM_NONE_EABI_GCC_PREFIX}gcc" AR="${ARM_NONE_EABI_GCC_PREFIX}ar" LD="${ARM_NONE_EABI_GCC_PREFIX}ld" CFLAGS='-Werror -Wall -Wextra -mthumb -mcpu=cortex-m0plus -Os' lib


Pre-existing: I'm surprised by the unexplained differences between this line and line 1593 above (in component_build_arm_none_eabi_gcc_armv5te): use of LD vs LDFLAGS, use of SHELL in armv5te.

The use of SHELL looks like a leftover from debugging.

Regarding LD vs LDFLAGS, I just checked our Makefiles and we never use LD (unless perhaps implicitly but I don't think we use any default rules): when we echo "LD ..." we actually invoke $(CC) ....

I suggest we uniformize the invocations of make in components using arm-none-eabi-gcc. But I understand if you'd rather to that in another PR.

SHELL should indeed not be there, but since that's preexisting, I propose to not touch it unless this part of PR needs rework anyway.

LD was added in e058ea2 and your guess is better than mine as to why.

Yes, I was afraid that would be the case. I guess I just thought it would be more consistent and failed to check what our Makefiles were actually using.

gilles-peskine-arm added mbed TLS team needs-work component-platform Portability layer and build scripts labels Apr 21, 2020

gilles-peskine-arm force-pushed the travis-rationalize branch from 8f078b1 to 2d82edf Compare April 21, 2020 21:18

mpg self-requested a review April 22, 2020 07:55

mpg reviewed Apr 22, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

gilles-peskine-arm force-pushed the travis-rationalize branch 6 times, most recently from b7f9962 to 21665c1 Compare April 23, 2020 21:50

gilles-peskine-arm added 9 commits April 25, 2020 22:34

all.sh: make the arm-gcc cross-compiler prefix configurable

60f267b

Make it possible to use a compiler that isn't in $PATH, or that's installed with a different name, or even a compiler for a different target such as arm-linux-gnueabi. Signed-off-by: Gilles Peskine <[email protected]>

Travis: move doxygen dependencies into the "sanity" job

b49a457

Only this job uses doxygen and graphviz. Signed-off-by: Gilles Peskine <[email protected]>

Travis: call all.sh for sanity checks

3c7ffd7

Different releases have different sets of sanity checks. Keep the list in one place, namely all.sh. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the travis-rationalize branch 2 times, most recently from d961ff4 to 48a27aa Compare April 25, 2020 21:49

gilles-peskine-arm added 4 commits April 26, 2020 00:16

Travis: install gnutls-bin for all jobs

a5ced5b

Some jobs don't actually test against GnuTLS, but all.sh checks its presence in all test jobs, so it needs to be installed regardless. Signed-off-by: Gilles Peskine <[email protected]>

Travis: add a build on macOS

129d041

Just do the default build with Clang and run the unit tests. The objective is to have one build on a Unix-like platform other than Linux. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the travis-rationalize branch from 48a27aa to ff88c3d Compare April 25, 2020 22:34

check-files: support Windows .bat files

af387e0

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the travis-rationalize branch from ff88c3d to 9cf4723 Compare April 25, 2020 22:44

mpg previously approved these changes Apr 27, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Apr 27, 2020

This was referenced Apr 27, 2020

Backport 2.16: Rationalize Travis builds #3247

Merged

Backport 2.7: Rationalize Travis builds #3248

Merged

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Apr 27, 2020

ronald-cron-arm reviewed Apr 28, 2020

View reviewed changes

gilles-peskine-arm dismissed mpg’s stale review via ce5b7b7 April 30, 2020 16:21

Rename --arm-gcc-prefix to --arm-none-eabi-gcc-prefix

6d06134

This is supposed to be for GCC (or a compiler with a compatible command line interface) targeting arm-none-eabi, so name it accordingly. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the travis-rationalize branch from ce5b7b7 to 6d06134 Compare April 30, 2020 16:41

gilles-peskine-arm added 4 commits April 30, 2020 23:14

all.sh: build_arm_none_eabi_gcc: do optimize

6537588

Otherwise the bignum assembly code is not used. Signed-off-by: Gilles Peskine <[email protected]>

all.sh: add a Cortex-M0+ build

6e2fb86

It's pretty fast and adds a little variety. Signed-off-by: Gilles Peskine <[email protected]>

all.sh: on arm builds (GCC or Arm Compiler), show the code size

18487f6

Just show the code size in the logs, for human consumption. Signed-off-by: Gilles Peskine <[email protected]>

Travis: do both a Cortex-A build and a Cortex-M0+ build

907211d

The Cortex-A build is similar to Debian armel. The Cortex-M0+ is a handy point of comparison for code size. Put that one last so that it's easy to find in the log. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm added the needs-ci Needs to pass CI tests label Apr 30, 2020

mpg requested review from mpg and ronald-cron-arm May 4, 2020 07:28

mpg removed the needs-ci Needs to pass CI tests label May 4, 2020

ronald-cron-arm reviewed May 4, 2020

View reviewed changes

mpg approved these changes May 4, 2020

View reviewed changes

gilles-peskine-arm mentioned this pull request May 4, 2020

Rationalize arm-gcc builds in all.sh #3299

Open

ronald-cron-arm approved these changes May 4, 2020

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels May 4, 2020

gilles-peskine-arm merged commit 39461b0 into Mbed-TLS:development May 4, 2020

gilles-peskine-arm mentioned this pull request Jun 25, 2020

Test outcome analysis: check that all available test cases have been executed #3458

Merged

tom-cosgrove-arm mentioned this pull request Mar 4, 2022

all.sh on Travis CI #2724

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rationalize Travis builds #3218

Rationalize Travis builds #3218

gilles-peskine-arm commented Apr 21, 2020 •

edited

Loading

mpg left a comment

gilles-peskine-arm commented Apr 27, 2020

ronald-cron-arm left a comment

ronald-cron-arm Apr 28, 2020

gilles-peskine-arm Apr 28, 2020

ronald-cron-arm Apr 30, 2020

gilles-peskine-arm Apr 30, 2020

ronald-cron-arm Apr 30, 2020

gilles-peskine-arm commented Apr 30, 2020

ronald-cron-arm left a comment

ronald-cron-arm May 4, 2020

gilles-peskine-arm May 4, 2020

ronald-cron-arm May 4, 2020

gilles-peskine-arm May 4, 2020

ronald-cron-arm May 4, 2020

mpg left a comment

mpg May 4, 2020

mpg May 4, 2020

gilles-peskine-arm May 4, 2020

mpg May 4, 2020

		- tests/scripts/test-ref-configs.pl
		- tests/scripts/all.sh -k build_arm_none_eabi_gcc_arm5vte build_arm_none_eabi_gcc_m0plus

Rationalize Travis builds #3218

Rationalize Travis builds #3218

Conversation

gilles-peskine-arm commented Apr 21, 2020 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 27, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 30, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 21, 2020 •

edited

Loading