Skip to content

Conversation

@ZetaR60
Copy link
Contributor

@ZetaR60 ZetaR60 commented Jul 5, 2018

Contribution description

This adds boards to BOARD_INSUFFICIENT_MEMORY for tests that failed due to memory errors on the new Docker image.

EDIT from cladmi: They also fail on the old docker image

Issues/PRs references

RIOT-OS/riotdocker#42
#9405

@ZetaR60 ZetaR60 added Area: tests Area: tests and testing framework Area: CI Area: Continuous Integration of RIOT components labels Jul 5, 2018
@kaspar030
Copy link
Contributor

Hm, why do some msp430s break with bionic? Isn't it using the same (age-old) toolchain?

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 5, 2018

According to packages.ubuntu.com it went from 4.6.3~mspgcc-20120406-7ubuntu3 to 4.6.3~mspgcc-20120406-7ubuntu5. Not sure what the exact meaning of the trailing number; a build number maybe?

@jnohlgard
Copy link
Member

Could be fstack-protector and _FORTIFY_SOURCE related. Try building for one of the failing platforms with CFLAGS=-fno-stack-protector

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 6, 2018

I added CFLAGS += -fno-stack-protector to the Makefiles of tests/pkg_fatfs_vfs and tests/pkg_u8g2 and they fail the same way when built with msb-430 or msb-430h.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 11, 2018

All of the applications that were failing before are now working according to make buildtest.

@kaspar030 @gebart Do you think that simply blocking the tests is acceptable in this circumstance? It might be possible to get them working again, but I lack the experience with the toolchains to effectively debug it myself. If we want the July release to be tested with the new Docker image, it may be easiest to use this PR and open a bug report on the broken packages (to be solved at some point in the future).

BOARD_BLACKLIST += chronos msb-430 msb-430h telosb wsn430-v1_3b wsn430-v1_4 z1

BOARD_INSUFFICIENT_MEMORY := nucleo-f031k6 nucleo-f042k6 nucleo-l031k6
BOARD_INSUFFICIENT_MEMORY := nucleo-f030r8 nucleo-f031k6 nucleo-f042k6 \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can help here: #9643

@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 27, 2018
endif

BOARD_INSUFFICIENT_MEMORY := nucleo-f031k6
BOARD_INSUFFICIENT_MEMORY := msb-430 msb-430h nucleo-f031k6 telosb \
Copy link
Contributor

Choose a reason for hiding this comment

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

I also have issue with my ubuntu stable for building these, I will try to find why.

@cladmi
Copy link
Contributor

cladmi commented Jul 27, 2018

I have the same issues with msp430 boards on my computer and in the riot docker container. So for me the last changes removing msp430 boards are valid.

For libfixmath and the nucleo-f030r8 it works on my computer, ubuntu stable, and arm gcc 7.3.1 but not in the docker container.

region `rom' overflowed by 1064 bytes

Which is a big difference… it should be investigated why. I will try to find what I can.

@jnohlgard
Copy link
Member

@cladmi perhaps your problem is related to #9643? Murdock paths may be longer than your local paths?

@cladmi
Copy link
Contributor

cladmi commented Jul 27, 2018

@gebart yes it is, in fact the relative path is not relative when building in docker #9645 (comment)

@cladmi
Copy link
Contributor

cladmi commented Jul 30, 2018

For pkg/fatfs there was already this issue here: #8408 and pkg_u8g2 fails on my computer (ubuntu xenial) and in the normal docker. So I would merge the pr with these two changes only.

sc_can is already merged and libfixmath root problem seems to be solved with my pr #9643

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 30, 2018

Reverted sc_can and libfixmath_unittests changes, leaving only pkg_fatfs_vfs and pkg_u8g2.

@cladmi
Copy link
Contributor

cladmi commented Jul 31, 2018

I will re-run tests locally with docker.

Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

Both tests work in docker for me. Please rebase and squash.

@ZetaR60 ZetaR60 force-pushed the RIOT_docker_compat branch from dbe529f to 25c7b3c Compare July 31, 2018 15:56
@miri64 miri64 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 Jul 31, 2018
@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 31, 2018

@miri64 I was waiting on Murdock as this is otherwise ready to merge. Please don't knock other peoples' PRs to the bottom of the queue unless it is really needed.

@cladmi
Copy link
Contributor

cladmi commented Jul 31, 2018

@ZetaR60 It's because murdock is not scheduling builds properly. The only work-around we found yet, is re-triggering every waiting PRs.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 31, 2018

Oh, okay.

@miri64 miri64 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 Jul 31, 2018
@miri64
Copy link
Member

miri64 commented Jul 31, 2018

Yeah sorry, @ZetaR60... I basically round-robin all the PRs in the queue right now, but I can give your PR precedence, if you like to.

@miri64 miri64 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 Jul 31, 2018
@cladmi cladmi merged commit 33389dd into RIOT-OS:master Jul 31, 2018
@cladmi cladmi added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jul 31, 2018
@cladmi
Copy link
Contributor

cladmi commented Jul 31, 2018

Backport provided in #9658

@cladmi cladmi changed the title tests/*: memory blacklisting for new Docker image tests/*: memory blacklisting for new Docker image (edit cladmi: and current image) Jul 31, 2018
@kaspar030
Copy link
Contributor

They also fail on the old docker image

Just to understand: we just accept that a toolchain upgrade increases RAM usage so certain applications don't fit anymore?

@cladmi
Copy link
Contributor

cladmi commented Aug 2, 2018

@kaspar030 You are generalizing and making it bigger than it is for this case. In practice it's many added reasons:

BOARD_INSUFFICIENT_MEMORY is only a list used so that the CI/buildtest/compile_test do not try to link applications that do not fit on its toolchain

  ifneq ($(filter $(BOARD_INSUFFICIENT_MEMORY), $(BOARD)),)
      $(info CI-build: skipping link step)
      RIOTNOLINK:=1
  endif

It does not mean it fits on your toolchain in general, it does not mean there will not be an issue with your toolchain if not in it.

toolchain upgrade

It has been non compiling in our current xenial docker image for 3 releases #8408, it is also not compiling on Ubuntu 16.04 as it failed during my release tests for Task 01.
Failing on the reference user platform is for me a sign that yes, linking could be disabled in CI if it is needed. The original issue is still open for the moment.
It may more be related to an increase of memory usage somewhere in RIOT and this broke here as they were already at limit memory usage.

Here my "needed" reason is, release tests fail because of it and in addition it is the only blocker for following ubuntu stable.

Also, these are two external packages, for really specific features, tests/pkg_fatfs_vfs requires an sdcard for testing and a lot of memory and u8g2 is a graphic display library.
These boards are already not linking unittests where the tests-core are located.

So yes because of all this, I disable linking them in CI.

@kaspar030
Copy link
Contributor

You are generalizing and making it bigger than it is for this case.

Sorry, I didn't intend to offend.

It may more be related to an increase of memory usage somewhere in RIOT and this broke here as they were already at limit memory usage.

This I don't understand. Why can Murdock link these just fine? This is not "increase of memory usage somewhere in RIOT", but caused by different toolchains, right? These things happen, but I think we need to get to the ground.

@miri64
Copy link
Member

miri64 commented Aug 2, 2018

Why can Murdock link these just fine?

Maybe related? #9398 (comment)

@cladmi
Copy link
Contributor

cladmi commented Aug 2, 2018

@kaspar030 Yeah also tired morning mood on my side, so was a bit grumpy, sorry :)

We have noticed difference of behavior between the docker image and murdock: #9645
Some are related to the build system docker integration like RIOT_FILE_RELATIVE not being relative in docker (wip pr #9646) but others should not be.

We have warnings only visible in the container for both #9362, #9605 and #9398 in general.

@cladmi cladmi added this to the Release 2018.07 milestone Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CI Area: Continuous Integration of RIOT components Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants