pkg: fix cross compiling with cmake on macOS by smlng · Pull Request #8509 · RIOT-OS/RIOT

smlng · 2018-02-02T09:50:18Z

Contribution description

This PR fixes cross compiling on macOS for packages that use cmake, these currently are ccn-lite and relic. While relic had this already I recently ran in troubles with ccn-lite, which is fixed here.

For future use by other packages the helper script is moved (and adapted a bit) to the tools directory, and Makefiles for both packages are adapted accordingly.

Issues/PRs references

kaspar030 · 2018-02-02T11:55:00Z

dist/tools/cmake/generate-xcompile-toolchain.pl

 print "SET(CMAKE_SYSTEM_VERSION 1)\n";
 print "\n";
+print "# specify the cross compiler\n";
+#print "SET(CMAKE_AR \"$ENV{AR}\" CACHE STRING \"\")\n";


here is something commented out

not needed, will remove

kaspar030 · 2018-02-02T11:55:30Z

pkg/ccn-lite/Makefile


-all: git-download
-	cd $(PKG_BUILDDIR)/src && cmake -DCCNL_RIOT=1 -DRIOT_CFLAGS="${RIOT_CFLAGS}" . && make
+$(info $(PKG_BUILDDIR))


debug output?

yep, forgot to remove

smlng · 2018-02-02T12:11:08Z

@kaspar030 fixed, amended directly

cladmi

Some makefile related remarks.
Also, please split the commit for relic in two between style changes and actual changes with xcompile.

cladmi · 2018-02-05T14:31:41Z

dist/tools/cmake/generate-xcompile-toolchain.pl

@@ -1,26 +1,21 @@
 #!/usr/bin/env perl


As moving to a general RIOT script, I would prefer to have it in either sh/bash or python than perl.

yeah thought about that, too, but started with using what was already in place with relic - I think sh would be best.

cladmi · 2018-02-05T14:33:46Z

makefiles/vars.inc.mk

 export RIOTCPU               # For third party CPUs this folder is the base of the CPUs.
 export RIOTBOARD             # For third party BOARDs this folder is the base of the BOARDs.
 export RIOTPKG               # For overriding RIOT's pkg directory
+export RIOTTOOLS             # For overriding RIOT's tools directory


Could this be done in a separate PR/issue, because right now there are still a lot of dist/tools in the repository, and only two packages using the variable does not help keeping things consistent:

git grep 'dist/tools' | wc -l 219

well that search mostly hits dist/tools in comments and readme docs, nevertheless you got a point, that this is sub-optimal and inconsistent. I'll look into it.

most hits are not actual usages but rather comments, e.g. in README.

There are still a lot, all the python tests use it for example.

git grep dist/tools | grep -v -e README -e doc.txt -e.md | wc -l
176

cladmi · 2018-02-05T14:41:25Z

pkg/ccn-lite/Makefile

+	$(MAKE) -C $(PKG_BUILDDIR)/src && \
 	cp $(PKG_BUILDDIR)/src/lib/libccnl-riot.a ${BINDIR}/ccn-lite.a

+$(PKG_BUILDDIR)/src/Makefile: $(PKG_BUILDDIR)/xcompile-toolchain.cmake


Please use a variable for $(PKG_BUILDDIR)/xcompile-toolchain.cmake its repeated 3 times in the build.

cladmi · 2018-02-05T14:41:46Z

pkg/ccn-lite/Makefile

-all: git-download
-	cd $(PKG_BUILDDIR)/src && cmake -DCCNL_RIOT=1 -DRIOT_CFLAGS="${RIOT_CFLAGS}" . && make
+all: $(PKG_BUILDDIR)/src/Makefile
+	$(MAKE) -C $(PKG_BUILDDIR)/src && \


&& not required here.

cladmi · 2018-02-05T14:42:50Z

pkg/ccn-lite/Makefile

+$(PKG_BUILDDIR)/src/Makefile: $(PKG_BUILDDIR)/xcompile-toolchain.cmake
+	cd $(PKG_BUILDDIR)/src && \
+	cmake -DCMAKE_TOOLCHAIN_FILE=$(PKG_BUILDDIR)/xcompile-toolchain.cmake \
+		  -DCCNL_RIOT=1 -DRIOT_CFLAGS="${RIOT_CFLAGS}" .


You could remove the cd by using $(@D) instead of .

I'm unsure, because IIRC cmake generates its intermediate files where it is called from, which in this case must be $(PKG_BUILDDIR)/src otherwise the make if above won't work.

You are right, I forgot cmake was working this way.

cladmi · 2018-02-05T14:43:08Z

pkg/ccn-lite/Makefile

+
+$(PKG_BUILDDIR)/xcompile-toolchain.cmake: git-download
+	cd $(PKG_BUILDDIR) && \
+	perl $(RIOTTOOLS)/cmake/generate-xcompile-toolchain.pl > xcompile-toolchain.cmake


Here too, you could just use $@ instead of cd.

cladmi · 2018-02-05T14:44:06Z

pkg/relic/Makefile

+
 all: $(PKG_BUILDDIR)/Makefile
-	"$(MAKE)" -C $(PKG_BUILDDIR) && \
+	$(MAKE) -C $(PKG_BUILDDIR) && \


Removed the " around MAKE, also && not required.

cladmi · 2018-02-05T14:46:24Z

pkg/relic/Makefile

-CFLAGS += -Wno-gnu-zero-variadic-macro-arguments -Wno-unused-function -Wno-newline-eof
+$(PKG_BUILDDIR)/xcompile-toolchain.cmake: fix_source
+	cd $(PKG_BUILDDIR) && \
+	perl $(RIOTTOOLS)/cmake/generate-xcompile-toolchain.pl > xcompile-toolchain.cmake


Same remarks as the other package with cd, I now understand where they come from.

cladmi · 2018-02-05T14:51:46Z

Post review thoughts. In fact, I could fix my make remarks in another PR if you want, as you basically just duplicated what was in relic in ccn-lite.

smlng · 2018-02-05T16:13:30Z

@cladmi I tried to address you comments, I changed the perl script into a shell script. Your hint with using @D didn't work for me, and removing the && \ didn't either <- but maybe its this (weird) Makefile scripting syntax. Regarding the RIOTTOOLS variable, it comes quite handy here, but I also don't want to break other stuff right now. So adapting other usages throughout RIOT source should be done in followup PRs, or not?

smlng · 2018-02-23T17:21:43Z

ping! May I squash?

smlng · 2018-02-28T13:50:27Z

ping again, @cladmi and @kaspar030 IMHO your change requests are addressed.

cladmi

Still would like the RIOTTOOLS done in another PR as it has no link to fix cros compiling with cmake.

Then remaining one minor variable that could use $(TOOLCHAIN_FILE), see inline.

I ignored makefile cleanups that where not done before already.

cladmi · 2018-03-22T14:31:09Z

pkg/relic/Makefile


-$(PKG_BUILDDIR)/comp-options.cmake: fix_source
-	cd "$(PKG_BUILDDIR)" && perl $(PKG_DIR)/generate-cmake-xcompile.perl > comp-options.cmake
+$(PKG_BUILDDIR)/Makefile: $(PKG_BUILDDIR)/xcompile-toolchain.cmake


This one should be $(TOOLCHAIN_FILE) too.

cladmi · 2018-03-22T14:35:09Z

What is the simple test procedure to test on mac if @kYc0o wants to check ?

smlng · 2018-03-27T17:05:47Z

@cladmi I removed RIOTTOOLS, saw you already provided a PR for that -> nice, thanks! If @kYc0o wants to verify: try to compile examples/ccn-lite on macOS for samr21-xpro linking will fail on master without this PR.

smlng · 2018-03-27T17:06:12Z

I think this is ready to merge now.

kaspar030 · 2018-03-28T11:42:29Z

pkg/ccn-lite/Makefile

+		  -DCCNL_RIOT=1 -DRIOT_CFLAGS="${RIOT_CFLAGS}" -DBUILD_TESTING=OFF .
+
+$(TOOLCHAIN_FILE): git-download
+	$(RIOTBASE)/dist/tools/cmake/generate-xcompile-toolchain.sh > $(TOOLCHAIN_FILE)


$(RIOTBASE)/dist/tools/cmake/generate-xcompile-toolchain.sh > $@

ah, Makefile magic - kind of :) will add

mhm, doesn't work though - $@ is expanded to /xcompile-toolchain.cmake only, without pkg build dir set.

so is $(TOOLCHAIN_FILE)? $@ should evaluate to the target name.

I'm not sure whats going wrong fact is:

$(RIOTBASE)/dist/tools/cmake/generate-xcompile-toolchain.sh > $@

is expanded to

/path/to/RIOT/dist/tools/cmake/generate-xcompile-toolchain.sh > /xcompile-toolchain.cmake

which obviously gives an error, because it is not allow to write to my root dir.

so the pkgbuilddir is empty at this point

leaving it as is, works

ok, thanks for trying. Weird that it can match the target but doesn't evaluate $@ afterwards?

At that time PKG_BUILDDIR is not defined because pkg.mk has not been included.
Goals are evaluated immediately not deferred. So the rule is really /xcompile-toolchain.cmake.

But the problem was also in the pkg/relic/Makefile before, so would require a real fix that could come later.

kaspar030 · 2018-03-28T11:43:05Z

pkg/relic/Makefile

-
-CFLAGS += -Wno-gnu-zero-variadic-macro-arguments -Wno-unused-function -Wno-newline-eof
+$(TOOLCHAIN_FILE): fix_source
+	$(RIOTBASE)/dist/tools/cmake/generate-xcompile-toolchain.sh > $(TOOLCHAIN_FILE)


kaspar030 · 2018-03-28T12:06:21Z

I think too this has waited long enough.

@smlng feel free to squash in the "$@".

@cladmi you happy here?

smlng requested review from cgundogan, kYc0o and tfar February 2, 2018 09:50

kaspar030 requested changes Feb 2, 2018

View reviewed changes

smlng force-pushed the cmake/xcompile branch from 156e211 to 4005ca8 Compare February 2, 2018 12:10

cladmi suggested changes Feb 5, 2018

View reviewed changes

smlng force-pushed the cmake/xcompile branch from 4005ca8 to bb56749 Compare February 5, 2018 16:08

smlng 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 Feb 7, 2018

smlng 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 Feb 21, 2018

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 28, 2018

cladmi suggested changes Mar 22, 2018

View reviewed changes

cladmi mentioned this pull request Mar 22, 2018

make: add and export RIOTTOOLS directory #8821

Merged

9 tasks

smlng added 4 commits March 27, 2018 19:00

make: export RANLIB tool with prefix

069f920

tools: add helper script for xcompile with cmake

a777f6f

pkg/ccn-lite: fix xcompile with cmake on macOS

3ad07c6

pkg/relic: adapt cmake xcompile on macOS

0e26cf8

smlng force-pushed the cmake/xcompile branch from bb56749 to 0e26cf8 Compare March 27, 2018 17:04

kaspar030 requested changes Mar 28, 2018

View reviewed changes

kaspar030 approved these changes Mar 28, 2018

View reviewed changes

cladmi approved these changes Mar 29, 2018

View reviewed changes

cladmi merged commit b502637 into RIOT-OS:master Mar 29, 2018

smlng deleted the cmake/xcompile branch March 29, 2018 12:20

cladmi mentioned this pull request Apr 10, 2018

make: use of immediate value of variables before they have their final value #8913

Open

6 tasks

Conversation

smlng commented Feb 2, 2018

Contribution description

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Feb 2, 2018

Uh oh!

cladmi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Feb 5, 2018

Uh oh!

smlng commented Feb 5, 2018

Uh oh!

smlng commented Feb 23, 2018

Uh oh!

smlng commented Feb 28, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 22, 2018

Uh oh!

smlng commented Mar 27, 2018

Uh oh!

smlng commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi left a comment •

edited

Loading