boards/*: remove unused FEATURES_MCU_GROUP variable #11670

aabadie · 2019-06-11T06:47:37Z

Contribution description

This variable is only used by drone.yml file and we don't use drone.io as CI anymore. Maybe the .drone.yml could also be removed ?

For the record, the Makefile.include of related boards were updated using the following command:

find . -name Makefile.features | xargs sed -i '/^# The board M[P,C]U family/,+2d'

Testing procedure

A green CI should be ok.

Issues/PRs references

fixes #10491
Waiting on #11672 as the way to handle the error message needs special handling with mac.

kaspar030 · 2019-06-11T08:56:10Z

we don't use drone.io as CI anymore. Maybe the .drone.yml could also be removed ?

IIRC think @vincent-d uses Drone internally?
Maybe the Drone file can be changed not to use the variable anymore.

cladmi · 2019-06-11T08:56:32Z

This was also referenced in #10491

This could also go with a removal/cleanup of BUILDTEST_MCU_GROUP if it is unused too.

aabadie · 2019-06-11T08:58:43Z

This was also referenced in #10491

Thanks for the pointer, I didn't find it initially.

IIRC think @vincent-d uses Drone internally?
Maybe the Drone file can be changed not to use the variable anymore.

Agreed

aabadie · 2019-06-11T09:00:22Z

This could also go with a removal/cleanup of BUILDTEST_MCU_GROUP if it is unused too.

git grep returns some use of BUILDTEST_MCU_GROUP with Murdock.

cladmi · 2019-06-11T09:13:47Z

This could also go with a removal/cleanup of BUILDTEST_MCU_GROUP if it is unused too.

git grep returns some use of BUILDTEST_MCU_GROUP with Murdock.

Only to call the static-tests part of ci/build_and_test.sh, which would better be in dist/tools/static-tests.sh directly.

It would need some refactoring and is not a simple removal as this one.

aabadie · 2019-06-11T09:19:47Z

It would need some refactoring and is not a simple removal as this one.

Indeed, let's leave this to a follow-up PR.

vincent-d · 2019-06-11T09:38:35Z

IIRC think @vincent-d uses Drone internally?

I don't ;)

cladmi · 2019-06-11T09:45:07Z

Indeed, let's leave this to a follow-up PR.

It is what I was thinking too, but was not clear while writing it.

cladmi · 2019-06-11T10:00:34Z

Removing this variable was also in my targets, so is good on my side. It just depends on the CI constraints.

I will a follow up PR to prevent-the variable to be re-included by a non updated PR.

cladmi · 2019-06-11T10:07:06Z

The follow up for preventing the variable to re-appear #11671

It showed that boards/same54-xpro must also be cleaned up.

If you want you can take the second commit alone and use it in this PR directly.

aabadie · 2019-06-11T10:26:13Z

If you want you can take the second commit alone and use it in this PR directly.

That's what I'll do.

Since @vincent-d is not using drone.io, I guess the file could be completely removed ?

cladmi · 2019-06-11T10:29:54Z

dist/tools/buildsystem_sanity_check/check.sh

+    pathspec+=(":!${SCRIPT_PATH}")
+
+    git -C "${RIOTBASE}" grep "${patterns[@]}" -- "${pathspec[@]}" \
+        | sed '1i Deprecated variables or patterns:'


I added this change in a fixup commit in #11671

Suggested change

| sed '1i Deprecated variables or patterns:'

| sed '1i \\nDeprecated variables or patterns:'

It is required to behave correctly when multiple errors are shown.

I cherry-picked the fixup commit

aabadie · 2019-06-11T10:33:14Z

dist/tools/buildsystem_sanity_check/check.sh

@@ -87,12 +87,31 @@ check_not_exporting_variables() {
    fi
 }

+# Deprecated variables or patterns
+# Prevent deprecated varibles or patterns to re-appear after cleanup


s/varibles/variables/

I just fixed it

cladmi · 2019-06-11T11:21:03Z

boards/same54-xpro must also be cleaned up, and it shown by the static tests \o/

cladmi · 2019-06-11T15:33:23Z

You can already squash the fixups and start a round on CI.

For the commits order, I would rather remove the usage first explaining why it is removed and then remove the definitions.
Otherwise the first commit removes the definitions while still being referenced and without justification.

Otherwise I agree with removing them, and if CI works without it, its good for me.

aabadie · 2019-06-11T15:37:40Z

Thanks @cladmi. I squashed and reordered the commits like you suggested. Let's see what Murdock has to say.

cladmi · 2019-06-13T10:12:40Z

The first commit documentation is not adapted.

    makefiles/info-global: remove use of FEATURES_MCU_GROUP
    
    The whole ifdef block depends on the fact that the FEATURES_MCU_GROUP variable is set but there
    no more occurence of it in the codebase. We can safely remove it.

The whole ifdef is depending on BUILDTEST_MCU_GROUP, not on FEATURES_MCU_GROUP. This disables the test filtering based on BUILDTEST_MCU_GROUP. Which will deprecate FEATURES_MCU_GROUP.

And after re-ordering, the definition are currently still there.

aabadie · 2019-06-13T11:12:10Z

The first commit documentation is not adapted.

I changed it.

cladmi · 2019-06-13T12:02:58Z

@kaspar030 @smlng from the CI point of view you agree this can be removed ?

smlng · 2019-06-13T13:14:35Z

to me this was inflexible anyway, and I think its not used anywhere anymore

cladmi · 2019-06-14T09:11:04Z

dist/tools/buildsystem_sanity_check/check.sh

+    pathspec+=(":!${SCRIPT_PATH}")
+
+    git -C "${RIOTBASE}" grep "${patterns[@]}" -- "${pathspec[@]}" \
+        | sed '1i \\nDeprecated variables or patterns:'


@aabadie #11672 was merged and changed the way to do the "buildsystem_sanity_check". Can you please rebase and adapt ?

Suggested change

| sed '1i \\nDeprecated variables or patterns:'

| error_with_message 'Deprecated variables or patterns:'

cladmi · 2019-06-14T09:11:22Z

dist/tools/buildsystem_sanity_check/check.sh


 main() {
    local errors=''

    errors+="$(check_not_parsing_features)"
    errors+="$(check_not_exporting_variables)"
+    errors+="$(check_deprecated_vars_patterns)"


The function should now be added the list in all_checks.

This is the last use of FEATURES_MCU_GROUP variable and thus it deprecates it.

Add a function to list deprecated variables or patterns and use it for * FEATURES_MCU_GROUP

aabadie · 2019-06-14T09:35:49Z

@cladmi, rebased!

kaspar030 · 2019-06-14T09:38:37Z

git grep returns some use of BUILDTEST_MCU_GROUP with Murdock.

Only to call the static-tests part of ci/build_and_test.sh, which would better be in dist/tools/static-tests.sh directly.

It would need some refactoring and is not a simple removal as this one.

has this been addressed?

aabadie · 2019-06-14T09:46:37Z

has this been addressed?

Is it supposed to be addressed here ? From previous @cladmi's comment, this is unclear to me.

kaspar030 · 2019-06-14T09:52:59Z

Is it supposed to be addressed here ? From previous @cladmi's comment, this is unclear to me.

Well, the static-tests are still run for this PR, and IIRC that's the only thing where the buildgroups variable was used by murdock, so I think we're good!

cladmi · 2019-06-14T09:59:12Z

BUILDTEST_MCU_GROUP is different from FEATURES_MCU_GROUP which is removed by this PR.

It currently is only used with BUILDTEST_MCU_GROUP=static-tests.

cladmi

ACK, good to remove it for me.

No counter arguments from the CI maintainers.

cladmi · 2019-06-17T10:41:32Z

Static test is still valid with last master, checked locally.

aabadie · 2019-06-17T11:47:33Z

Thanks!

aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CI Area: Continuous Integration of RIOT components labels Jun 11, 2019

aabadie requested review from cladmi and kaspar030 June 11, 2019 06:47

aabadie force-pushed the pr/boards/remove_features_mcu_group branch from bf1186c to 063350d Compare June 11, 2019 09:18

aabadie force-pushed the pr/boards/remove_features_mcu_group branch from 063350d to 7d908f6 Compare June 11, 2019 09:24

cladmi mentioned this pull request Jun 11, 2019

dist/tools/build_system_sanity_check: deprecate FEATURES_MCU_GROUP #11671

Closed

3 tasks

cladmi reviewed Jun 11, 2019

View reviewed changes

aabadie commented Jun 11, 2019

View reviewed changes

aabadie force-pushed the pr/boards/remove_features_mcu_group branch from bd1173d to 2440ac4 Compare June 11, 2019 15:36

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 11, 2019

aabadie force-pushed the pr/boards/remove_features_mcu_group branch from 2440ac4 to 3c362a8 Compare June 13, 2019 11:11

cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 13, 2019

cladmi reviewed Jun 14, 2019

View reviewed changes

aabadie and others added 4 commits June 14, 2019 11:32

makefiles/info-global: remove use of FEATURES_MCU_GROUP

27e5aa5

This is the last use of FEATURES_MCU_GROUP variable and thus it deprecates it.

.drone.yml: remove mcu groups from the build test matrix

bbee0a7

boards/*: remove unused FEATURES_MCU_GROUP variable

a68f78b

dist/tools/build_system_sanity_check: deprecate FEATURES_MCU_GROUP

7d65a71

Add a function to list deprecated variables or patterns and use it for * FEATURES_MCU_GROUP

aabadie force-pushed the pr/boards/remove_features_mcu_group branch from 3c362a8 to 7d65a71 Compare June 14, 2019 09:34

cladmi approved these changes Jun 17, 2019

View reviewed changes

cladmi merged commit a379d95 into RIOT-OS:master Jun 17, 2019

aabadie deleted the pr/boards/remove_features_mcu_group branch July 2, 2019 11:23

This was referenced Nov 2, 2022

dist/tools: Add compile_like_murdock #18803

Merged

.drone: Remove unmaintained .drone file #18832

Merged

	\| sed '1i Deprecated variables or patterns:'
	\| sed '1i \\nDeprecated variables or patterns:'

	\| sed '1i \\nDeprecated variables or patterns:'
	\| error_with_message 'Deprecated variables or patterns:'

boards/*: remove unused FEATURES_MCU_GROUP variable #11670

boards/*: remove unused FEATURES_MCU_GROUP variable #11670

Uh oh!

Conversation

aabadie commented Jun 11, 2019 • edited by cladmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

vincent-d commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

cladmi Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

cladmi commented Jun 13, 2019

Uh oh!

aabadie commented Jun 13, 2019

Uh oh!

cladmi commented Jun 13, 2019

Uh oh!

smlng commented Jun 13, 2019

Uh oh!

cladmi Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jun 14, 2019

Uh oh!

kaspar030 commented Jun 14, 2019

Uh oh!

aabadie commented Jun 14, 2019

Uh oh!

kaspar030 commented Jun 14, 2019

Uh oh!

cladmi commented Jun 14, 2019

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jun 11, 2019 •

edited by cladmi

Loading