Skip to content

dist/tools/build_system_sanity_check: BUG fix errors being ignored #11695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 26, 2019
Merged

dist/tools/build_system_sanity_check: BUG fix errors being ignored #11695

merged 2 commits into from
Jun 26, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jun 14, 2019 •
edited
Loading

Contribution description

On error messages the exit code was still 0 due to the typo.
Having error messages again properly return with an exit code of 1.

Fix error_on_input that was always returning an error too.

Testing procedure

With the test commit the static tests will fail (now CI: needs squashing is set).

Revert the revert commit be1551c18 and run the script, it will exit with a '1' exit code. In the current master it would return 0.

Issues/PRs references

Bug introduced by #11672 which led to having this test commit merged https://github.com/RIOT-OS/RIOT/pull/11694/files

@cladmi
dist/tools/build_system_sanity_check: BUG fix errors being ignored
b744ba7 
On error messages the exit code was still 0 due to the typo.
Having error messages again properly return with an exit code of 1.
@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Area: tools Area: Supplementary tools labels Jun 14, 2019
@cladmi cladmi added this to the Release 2019.07 milestone Jun 14, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Jun 14, 2019 •
edited
Loading

The error is correctly detected with the test commit.

https://travis-ci.org/RIOT-OS/RIOT/builds/545647185#L444

Running './dist/tools/headerguards/check.sh' ✓
Running './dist/tools/buildsystem_sanity_check/check.sh' x
Command output:
	Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
	Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
		makefiles/boot/riotboot.mk:ifneq (,$(filter riotboot,$(FEATURES_USED)))
		makefiles/boot/riotboot.mk:endif # (,$(filter riotboot,$(FEATURES_USED)))
		sys/Makefile.include:ifneq (,$(filter riotboot,$(FEATURES_USED)))
	Variables must not be exported:
		boards/pic32-clicker/Makefile.include:export APPDEPS += $(RIOTCPU)/$(CPU)/$(CPU_MODEL)/$(CPU_MODEL).S
		boards/pic32-wifire/Makefile.include:export APPDEPS += $(RIOTCPU)/$(CPU)/$(CPU_MODEL)/$(CPU_MODEL).S
		makefiles/vars.inc.mk:export APPDEPS               # Files / Makefile targets that need to be created before the application can be build. Set in the application's Makefile.
	Variables must only be exported in `makefiles/vars.inc.mk`:
		Makefile.include:export CCACHE_CPP2=yes
		makefiles/arch/mips.inc.mk:export CCAS = $(PREFIX)gcc
		makefiles/arch/mips.inc.mk:export CCASUWFLAGS += -target $(TARGET_ARCH)
		makefiles/toolchain/gnu.inc.mk:export CC         = $(PREFIX)gcc
		makefiles/toolchain/gnu.inc.mk:export CCAS      ?= $(CC)
		makefiles/toolchain/llvm.inc.mk:export CC          = clang
		makefiles/toolchain/llvm.inc.mk:export CCAS       ?= $(CC)
make: *** [static-test] Error 1
makefiles/tests.inc.mk:3: recipe for target 'static-test' failed

@cladmi cladmi force-pushed the pr/buildsystem_sanity_check/bug/never_returns_error branch from fcdd2f3 to b744ba7 Compare June 14, 2019 09:17
@cladmi cladmi removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 14, 2019
@cladmi cladmi requested review from jcarrano and kaspar030 June 14, 2019 09:18
@cladmi cladmi mentioned this pull request Jun 14, 2019
Closed
3 tasks
@jcarrano
Copy link
Contributor

D'oh! sorry, I screwed up. It was missing the "needs squash" label.

@cladmi
dist/tools/build_system_sanity_check: BUG error_on_input always fail
e4b82be 
The function was always failing but was hidden by the previous bug too.
@cladmi
Copy link
Contributor Author

cladmi commented Jun 14, 2019

And another BUG. I should have listened to me when I said, this should go in a separate PR and not piggyback it to the main one.

@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 Jun 14, 2019
jcarrano
jcarrano approved these changes Jun 26, 2019
View reviewed changes
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.

I followed the test procedure and it works.

@jcarrano jcarrano merged commit 6349d91 into RIOT-OS:master Jun 26, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Jun 26, 2019

Thank you. I will be more careful next time.

@cladmi cladmi deleted the pr/buildsystem_sanity_check/bug/never_returns_error branch June 26, 2019 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 Awaiting requested review from kaspar030

1 more reviewer

@jcarrano jcarrano jcarrano approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.07
Development

Successfully merging this pull request may close these issues.
2 participants
@cladmi @jcarrano