Re: [dpdk-dev] [PATCH v6 1/1] ci: Introduce travis builds for github repositories

[Michael Santana Francisco <msantana@redhat.com>]

Thank you for taking at look at this
On 3/20/19 12:01 PM, Thomas Monjalon wrote:
> Hi,
>
> 04/03/2019 17:12, Michael Santana:
>>   .ci/linux-build.sh                  | 21 +++++++++
>>   .ci/linux-setup.sh                  |  3 ++
>>   .travis.yml                         | 73 +++++++++++++++++++++++++++++
> Please, could you explain somewhere what is the relationship
> between these files?
> What is specific to Travis?
> What is specific to GitHub?
>
> May we add "travis-" as filename prefix of the scripts?
> Or rename .ci to .travis?
Only the .travis.yml is specific to travis. The other two files are used 
by travis, but are independent of travis.
This allows us for in the future to change travis as CI and use 
something else instead, or add another CI in addition to travis if we 
wanted to. Other CI's would just run these scripts just like travis does.
With that said, I would not change the names, but if you really think 
they should then it's not a problem
>> +++ b/.ci/linux-build.sh
>> @@ -0,0 +1,21 @@
>> +#!/bin/bash -xe
> If possible, I would prefer a simple /bin/sh.
>
>> +function on_error() {
>> +    FILES_TO_PRINT=( "build/meson-logs/testlog.txt" "build/.ninja_log" "build/meson-logs/meson-log.txt")
>> +
>> +    for pr_file in "${FILES_TO_PRINT[@]}"; do
> You can make FILES_TO_PRINT as a simple word list,
> and so avoid bashism.
Will look into it
>
> [...]
>> +if [ "${AARCH64}" == "1" ]; then
> Please explain in the comment where this variable comes from.
> I suggest renaming it to ARMV8 as this is what it is translated to:
The variable comes from travis. If you look at the matrix in the 
travis.yml you will see lines containing environment variables like 
AARCH64=1.
These lines tell the travis build job to explicitly export these 
variables so that they can be used by the CI scripts like this one.
As for ARMV8 someone had asked specifically to be named AARCH64
>
>> +    # convert the arch specifier
>> +    OPTS="${OPTS} -DRTE_ARCH_64=1 --cross-file config/arm/arm64_armv8_linuxapp_gcc"
> I think -DRTE_ARCH_64=1 is useless.
>
>> +fi
>> +
>> +OPTS="$OPTS --default-library=$DEF_LIB"
>> +meson build --werror -Dexamples=all ${OPTS}
>> +ninja -C build
> [...]
>> --- /dev/null
>> +++ b/.travis.yml
>> @@ -0,0 +1,73 @@
>> +language: c
>> +compiler:
>> +  - gcc
>> +  - clang
>> +
>> +dist: xenial
> Are we going to update the distribution frequently?
> Why not adding more distros?

The only Linux distribution travis supports is Ubuntu, and the latest 
Ubuntu version they support is xenial which is code name for Ubuntu 16.04

>
>> +os:
>> +  - linux
> Is it possible to run on FreeBSD?
Not that I am aware. Travis only supports Ubuntu, Windows, and Mac
>
>> +addons:
>> +  apt:
>> +    update: true
>> +    packages:
>> +      - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +
>> +before_install: ./.ci/${TRAVIS_OS_NAME}-setup.sh
>> +
>> +sudo: false
>> +
>> +env:
>> +  - DEF_LIB="static"
>> +  - DEF_LIB="shared"
>> +  - DEF_LIB="static" OPTS="-Denable_kmods=false"
>> +  - DEF_LIB="shared" OPTS="-Denable_kmods=false"
> How is it different of the matrix below?
> Why testing disabling kmods?
This list inherits the packages from the package list above.
The main difference is that this list does not contain extra packages 
such as libbsd-dev, libpcap-dev, etc, where as the matrix below does.
This is so that we have a series of builds with minimal libraries 
installed (Minimal build) and another series of builds with many 
libraries installed (Full build).
We want to mix and match all the possible build scenarios and see if a 
new changes breaks any of the build cases.
The more builds we have with the many different configurations we can 
have the wider the net we can cast to ensure everything is working as it 
should
>
>> +
>> +matrix:
>> +  include:
>> +  - env: DEF_LIB="static" OPTS="-Denable_kmods=false" AARCH64=1
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +          - [gcc-aarch64-linux-gnu, libc6-dev-arm64-cross]
> Why packages are repeated here again?
> (sorry, I don't know Travis and I want to understand)
Yeah, we don't want to repeat ourselves either but we have no choice. 
This is due to a limitation in travis.
This matrix does not inherit any packages from the main package list way 
above, which means we have to list them out manually here.
In addition to the required packages we also want to install full builds 
with libraries like libbsd-dev, libpcap-dev, etc.
We could of just put those libraries in the main package list above and 
put all the builds in the env: list because then the libraries would be 
inherited.
The problem with that is that is that travis would not keep minimal 
builds and full builds separate.
We could not have minimal builds because the minimal builds will also 
inherit the additional libraries; Meson will then automatically detect 
those additional libraries and builds with them.
What we would like to have is a way to tell meson which libraries we 
want to use and which we dont, instead of being auto-detected. This 
would help us to get rid of this matrix.

If someone knows a better way to do this we would greatly take in your 
ideas, but so far this is the best we could come up with
>
>> +  - env: DEF_LIB="shared" OPTS="-Denable_kmods=false" AARCH64=1
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +          - [gcc-aarch64-linux-gnu, libc6-dev-arm64-cross]
>> +  - env: DEF_LIB="static"
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4]
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +  - env: DEF_LIB="shared"
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4]
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +  - env: DEF_LIB="static" OPTS="-Denable_kmods=false"
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4]
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
>> +  - env: DEF_LIB="shared" OPTS="-Denable_kmods=false"
>> +    compiler: gcc
>> +    addons:
>> +      apt:
>> +        packages:
>> +          - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4]
>> +          - [libnuma-dev, linux-headers-$(uname -r), python3-setuptools, python3-wheel, python3-pip, ninja-build]
> It seems clang is not in the matrix. Why?
It looks like my mistake, will look into it
>
> Thanks for this v6.
> I will be available to follow more closely in next days,
> so we can merge this feature soon this week.
>
>