Skip to content

Conversation

@cladmi
Copy link
Contributor

@cladmi cladmi commented Dec 7, 2018

Contribution description

When building with BUILD_IN_DOCKER the compile-tests were also tried to be executed on the host machine which had concurrency issues when done in parallel and also fails if you do not have the toolchain.

I also inlined added a change that verify the test file exists as the test would silently compare the error string instead of the real output.

Testing procedure

Using the first commit only:

On a clean environment these both fail:

BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=samr21-xpro make -C tests/cortexm_common_ldscript/ -j clean all
BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=samr21-xpro make -C tests/libc_newlib/ -j clean all

And with the following, the tests are executed both in docker and the build machine

BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=samr21-xpro make -C tests/libc_newlib/ all
...
Test: comparing addresses of 'printf' and 'iprintf' symbols
[SUCCESS] '0000158c' = '0000158c' is True
Test: comparing addresses of 'printf' and 'iprintf' symbols
[SUCCESS] '0000158c' = '0000158c' is True
make: Leaving directory '/home/harter/work/git/RIOT/tests/libc_newlib'

Issues/PRs references

Found while building in docker on a machine without toolchain and also again during #10344 testing.

If the file does not exist, because of concurrency issues, there is no
error. So verify it is there for good measures.
Without the protection, the host system also tries to execute the tests.
Without the protection, the host system also tries to execute the tests.
@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: tests Area: tests and testing framework Area: build system Area: Build system Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … labels Dec 7, 2018
@cladmi cladmi requested a review from jcarrano December 7, 2018 16:50
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested both commands, all good.

@cladmi
Copy link
Contributor Author

cladmi commented Dec 7, 2018

I updated the test description as I pasted the same command twice for the first one.

Also I forgot to say that the fail case was with the first commit applied. Does not change what works, just how to show it failed.

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 8, 2018
@jcarrano jcarrano merged commit c644110 into RIOT-OS:master Dec 10, 2018
@cladmi cladmi deleted the pr/tests/compile_tests_in_docker branch December 13, 2018 12:55
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: build system Area: Build system Area: tests Area: tests and testing framework Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants