Skip to content
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.

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

Makefile.include: fix missing target for libraries #8911

Merged
merged 1 commit into from
Apr 17, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Apr 10, 2018

Unittests add libraries in 'BASELIBS' which do not have any rules to be built as
they are built by 'application.inc.mk' and the DIRS variable.
So make complains about missing target for the unittests archives.

The fix tells these files are generated when building '$(APPLICATION_MODULE).a'.

The bug was introduced by #8844

Fixes #8910

@cladmi cladmi force-pushed the pr/fix/makefile/include/unittests branch from 8e3f7ba to 049d4c2 Compare April 10, 2018 14:35
@cladmi cladmi changed the title Makefile.inlude: fix missing target for libraries Makefile.include: fix missing target for libraries Apr 10, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Apr 10, 2018

I will try a PR to show that murdock fails to detect it.

@bergzand
Copy link
Member

This fixes #8910 for me. I'm not familiar enough with the makefiles to review the code here though.

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Apr 10, 2018
@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 Apr 10, 2018
@cladmi cladmi requested a review from kaspar030 April 10, 2018 15:27
@cladmi
Copy link
Contributor Author

cladmi commented Apr 10, 2018

Even when doing make clean; make all it works, the commits for murdock will be removed before merging.

@emmanuelsearch can you test ?

@emmanuelsearch
Copy link
Member

make clean does help indeed.

@cladmi
Copy link
Contributor Author

cladmi commented Apr 10, 2018

The _SUBMAKE_LIBS is at that point not really defined to the files built by the application makefile because $(BASELIBS) does not have its final value.
But as the $(ELFFILE) rule also uses the same value, the fix works.

I added an issue to track this problem of use before define. #8913

@cladmi cladmi added this to the Release 2018.04 milestone Apr 12, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Apr 12, 2018

Fixed the rule because some modules are built by packages makefile.

@kaspar030
Copy link
Contributor

please squash!

Unittests add libraries in 'BASELIBS' which do not have any rules to be built as
they are built by 'application.inc.mk', packages and the DIRS variable.
So make complains about missing target for the unittests archives.

The fix tells these files are generated when building '$(APPLICATION_MODULE).a'.

The bug was introduced by RIOT-OS#8844

Fixes RIOT-OS#8910
@cladmi cladmi force-pushed the pr/fix/makefile/include/unittests branch from ad19d71 to 94214cd Compare April 16, 2018 10:55
@cladmi
Copy link
Contributor Author

cladmi commented Apr 16, 2018

Rebased and squashed message

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030 kaspar030 merged commit 32807bd into RIOT-OS:master Apr 17, 2018
@cladmi cladmi deleted the pr/fix/makefile/include/unittests branch April 17, 2018 15:22
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
…nittests

Makefile.include: fix missing target for libraries
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 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
Development

Successfully merging this pull request may close these issues.

tests: Unittests don't build
4 participants