make: cleanup and centralise DEVELHELP setting by smlng · Pull Request #8029 · RIOT-OS/RIOT

smlng · 2017-11-14T10:19:37Z

This PR make the usage of DEVELHELP more consistent and easier to configure (IMHO 😄), i.e.,
its no longer required to explicitly set CFLAGS += -DDEVELHELP in Makefiles but rather use DEVELHELP=1 either in Makefile or as env variable on command line; specifically for the latter its simpler.

Further this PR make usage of DEVELHELP more consistent, as follows:

remove setting of DEVELHELP for all examples, can be optionally activated as described above
enable DEVELHELP for all test by default, in tests/Makefile.test_common

smlng · 2017-11-14T10:37:18Z

requires #8030

cladmi

I like the idea, it adds a real configuration for "debug". It is not anymore a CFLAGS you set on your own.

Should also the NDEBUG configuration be adapted to use DEVELHELP instead of parsing CFLAGS ? https://github.com/RIOT-OS/RIOT/blob/2017.10-branch/makefiles/cflags.inc.mk#L64

cladmi · 2017-11-14T10:40:04Z

tests/openthread/Makefile

-# Comment this out to disable code in RIOT that does safety checking
-# which is not needed in a production environment but helps in the
-# development process:
-CFLAGS += -DDEVELHELP -Wall


-Wall got removed at the same time here

IMHO wasn't appropriate place to activate that here anyway, use WERROR (see e.g. #7919) instead

cladmi · 2017-11-14T10:41:32Z

Makefile.include


+# Set this to 1 to enable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:


You could also add that it enables the DEBUG_PRINT macro. https://github.com/RIOT-OS/RIOT/blob/2017.10-branch/core/include/debug.h#L46

kaspar030 · 2017-11-14T10:56:26Z

I like!

remove setting of DEVELHELP for all examples, can be optionally activated as described above

I think this is a rather big change and should maybe be kept out of this PR.

smlng · 2017-11-14T10:59:13Z

I think this is a rather big change and should maybe be kept out of this PR.

mhm, it wasn't used consistently and also makes not much sense to have DEVELHELP active for examples by default (IMHO). Iff we want to have develhelp active nevertheless I would still adapt it here to set DEVELHELP = 1 in respective Makefile of the examples, instead of keeping the CFLAG variant

kaspar030 · 2017-11-14T11:00:46Z

would still adapt it here to set DEVELHELP = 1 in respective Makefile of the examples

I think that would be the way to go!

smlng · 2017-11-14T11:01:15Z

oh great, Murdock reveals failed assertion in gnrc:

main(): This is RIOT! (Version: buildtest)
...................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c:772 => 0x8049ba9
*** RIOT kernel panic:
FAILED ASSERTION.

and further

gnrc_sixlowpan_nd_router.c: In function 'gnrc_sixlowpan_nd_router_abr_rem_prf':
gnrc_sixlowpan_nd_router.c:330:62: error: unused parameter 'iface' [-Werror=unused-parameter]
                                           gnrc_ipv6_netif_t *iface, gnrc_ipv6_netif_addr_t *prefix)
                                                              ^

so my PR will be blocked by this (IMHO nonsense) GNRC merge embargo 😞

miri64 · 2017-11-14T11:05:31Z

Can we let DEVELHELP activated on Murdock? Or is that not desirable for future size monitoring?

miri64 · 2017-11-14T11:06:24Z

(otherwise: should be pretty straight forward by setting DEVELHELP=1 in .murdock, right?

kaspar030 · 2017-11-14T11:08:09Z

Or is that not desirable for future size monitoring?

I'd say murdock shouldn't compile differently from what the Makefile says, if possible. Also, for size monitoring, DEVELHELP shouldn't be set. (At least for tests/minimal, examples/hello-world, examples/gnrc_minimal ...)

smlng · 2017-11-14T11:08:25Z

regarding murdock, I think we should compile everything with and without DEVELHELP=1, as it checks out differently - at least for the nightlies we should do that.

kaspar030 · 2017-11-14T11:09:15Z

enable DEVELHELP for all test by default, in tests/Makefile.test_common

Can you disable it for tests/minimal?

kaspar030 · 2017-11-14T11:11:10Z

DEVELHELP shouldn't be set. (At least for tests/minimal, examples/hello-world, examples/gnrc_minimal ...)

Sorry, I meant, "shouldn't be changed"

smlng · 2017-11-14T17:39:55Z

@kaspar030 I excluded tests/minimal from DEVELHELP

smlng · 2017-11-14T17:41:05Z

I still [think] DEVELHELP doesn't make sense for examples by default

cladmi · 2017-11-15T17:06:22Z

I would prefer this separated in two PRs:

Introduce the new configuration mechanism
Sanitize the usage of DEVELHELP

smlng · 2017-11-17T18:01:14Z

factored out changes as requested, this now only introduces the new develhelp switch.

PR depends on #8030.

And is needed by #8079, #8080, and #8081

kaspar030 · 2017-11-17T21:41:09Z

PR depends on #8030.

Sorry, but why?

kaspar030 · 2017-11-17T21:46:37Z

only introduces the new develhelp switch.

I'm sorry to be picky on this, but the switch also has to be used within the makefiles, instead of setting CFLAGS directly.

smlng · 2017-11-18T08:47:41Z

PR depends on #8030.
Sorry, but why?

Right, this one doesn't need it but the #8080 does, otherwise tests/pkg_libcoap fails. Will drop here.

smlng · 2017-11-18T08:48:38Z

only introduces the new develhelp switch.
I'm sorry to be picky on this, but the switch also has to be used within the makefiles, instead of setting CFLAGS directly.

I don't understand, you said the change should be factored out, hence #8080 and #8081. Or what do you mean?

miri64

Can you please update the information in doc.txt with some information on the environment variable as well, please?

smlng · 2017-11-18T18:49:48Z

good point @miri64, will do!

kaspar030

ACK.

smlng · 2017-11-20T14:15:23Z

@miri64 the doc is rather generic already, I don't see any point in fixing it - but could be extended?!

miri64 · 2017-11-20T14:53:46Z

I don't see any point in fixing it - but could be extended?!

Extension was what I was asking for. At least with regards to this PR. Something like the sentence "Can be set by setting environment variable DEVELHELP is set to 1 at compile time." would already satisfy me.

Was added in the documentation.

kaspar030 · 2017-11-23T13:20:55Z

@miri64 you fine with the extra doc?

miri64 · 2017-11-23T13:22:25Z

doc.txt

+ *
+ *          Additional code parts such as (extensive) debug output and sanity
+ *          checks using assertions. To activate set environment variable
+ *          `DEVELHELP=1`, or disable (if on by default) with `DEVELHELP=0`.


the "if" looks a little bit out of place here.

smlng · 2017-11-23T17:14:38Z

@miri64 changed wording, better?

miri64

Yes, ACK.

smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Nov 14, 2017

smlng requested a review from kaspar030 November 14, 2017 10:19

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 14, 2017

smlng mentioned this pull request Nov 14, 2017

pkg/libcoap: adapt Makefile to fix compile error #8030

Merged

smlng mentioned this pull request Nov 14, 2017

pkg/tinydtls: update version to fix compiler errors #8031

Merged

cladmi previously requested changes Nov 14, 2017

View reviewed changes

smlng force-pushed the enh/make/develhelp branch from 8bc85b7 to be1a2cb Compare November 14, 2017 10:49

smlng force-pushed the enh/make/develhelp branch from be1a2cb to 79d5ed8 Compare November 14, 2017 11:04

smlng force-pushed the enh/make/develhelp branch 4 times, most recently from 375abc7 to 7df250b Compare November 14, 2017 15:45

smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 14, 2017

miri64 assigned cladmi Nov 16, 2017

This was referenced Nov 17, 2017

dist: adapt develhelp in Makefile #8079

Merged

make: enable develhelp for all tests, except minimal #8080

Merged

make: adapt develhelp for all examples #8081

Merged

smlng force-pushed the enh/make/develhelp branch from 374f888 to 533b676 Compare November 18, 2017 08:49

miri64 requested changes Nov 18, 2017

View reviewed changes

kaspar030 approved these changes Nov 20, 2017

View reviewed changes

make: allow for DEVELHELP env variable

a7bc9af

smlng force-pushed the enh/make/develhelp branch from 533b676 to 8a677dc Compare November 20, 2017 15:05

cladmi approved these changes Nov 21, 2017

View reviewed changes

miri64 reviewed Nov 23, 2017

View reviewed changes

doc: describe usage of DEVELHELP

a05032f

smlng force-pushed the enh/make/develhelp branch from 8a677dc to a05032f Compare November 23, 2017 17:13

miri64 approved these changes Nov 23, 2017

View reviewed changes

miri64 merged commit ba46317 into RIOT-OS:master Nov 23, 2017

smlng deleted the enh/make/develhelp branch November 23, 2017 17:39

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

Conversation

smlng commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi Nov 14, 2017

Choose a reason for hiding this comment

Uh oh!

smlng Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

cladmi Nov 14, 2017

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017

Uh oh!

kaspar030 commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017

Uh oh!

miri64 commented Nov 14, 2017

Uh oh!

miri64 commented Nov 14, 2017

Uh oh!

kaspar030 commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017

Uh oh!

kaspar030 commented Nov 14, 2017

Uh oh!

kaspar030 commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017

Uh oh!

smlng commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Nov 17, 2017

Uh oh!

kaspar030 commented Nov 17, 2017

Uh oh!

smlng commented Nov 18, 2017

Uh oh!

smlng commented Nov 18, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 18, 2017

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 20, 2017

Uh oh!

miri64 commented Nov 20, 2017

Uh oh!

kaspar030 commented Nov 23, 2017

Uh oh!

miri64 Nov 23, 2017

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 23, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

smlng commented Nov 14, 2017 •

edited

Loading

cladmi commented Nov 15, 2017 •

edited

Loading

smlng commented Nov 17, 2017 •

edited

Loading