pkg/ccn-lite/relic: check for minimal cmake version#9720
pkg/ccn-lite/relic: check for minimal cmake version#9720miri64 merged 3 commits intoRIOT-OS:masterfrom
Conversation
cladmi
added
the
CI: ready for build
|
@smlng I asked you to also have a test run on |
|
@cladmi, some packet used in |
|
I thought about it just after submitting it >< |
pkg/ccn-lite/Makefile
Outdated
| cp $(PKG_BUILDDIR)/src/lib/libccnl-riot.a $(BINDIR)/ccn-lite.a | ||
|
|
||
| $(PKG_BUILDDIR)/src/Makefile: $(TOOLCHAIN_FILE) | ||
| $(PKG_BUILDDIR)/src/Makefile: $(TOOLCHAIN_FILE) | ..cmake_version_supported |
There was a problem hiding this comment.
This is executed with every run without being cached, right? Can't we touch a file to skip subsequent version checks for an existing binary dir?
There was a problem hiding this comment.
Here TOOLCHAIN_FILE is also rebuilt everytime as it depends on git-download and so is Makefile created by cmake…
A solution would still require to save command -v cmake to a file and verify it did not change since last call.
So I would more do it after I can finish working on #9634.
There was a problem hiding this comment.
can't you run the cmake version check before downloading or generating the toolchain file?
There was a problem hiding this comment.
I was more concentrated on getting a working test than where I should put it.
Putting it as a check before downloading could be problematic if download gets done at a separate time or even a different environment. I still hope for a separate "prepare" step. But it could be moved again later.
So yes I can move it.
|
Doesn't work on macOS, I have cmake 3.12.0: but I get the error: Note: the |
|
I have a quoting issue in the command, does this work ? |
|
@cladmi the |
|
The issue for I will move the test to a dedicated script to allow reusing it. |
|
Problem when doing the check before downloading, is that So the error message is shown two times… |
|
It still prevents to download the source though so could be kept this way. |
| @@ -0,0 +1,33 @@ | |||
| #! /bin/bash | |||
| # | |||
| # usage: has_higher_version.sh <version> <minimal_version> [toolname] | |||
There was a problem hiding this comment.
wouldn't has_minimal_version be semantically (more) correct because the version doesn't have to be higher/greater but greater or equal, hence it should at least match the minimal version given
There was a problem hiding this comment.
I will rename the scripts.
There was a problem hiding this comment.
I also used "minimal version" everywhere but was not able to name the script accordingly ><
|
|
||
|
|
||
| HIGHEST=$(printf "%s\n%s\n" "${TOOLVERSION}" "${MINVERSION}" | sort -rV | head -n 1) | ||
| test "${HIGHEST}" = "${TOOLVERSION}" && exit 0 |
There was a problem hiding this comment.
would be nice to have a positiv message if version is good, too.
There was a problem hiding this comment.
I would not output it in the build system. We say nothing about "gcc has a good version", "make is up to date", "wget is compatible".
Or I could print something to standard output and silent the error in Make.
| HIGHEST=$(printf "%s\n%s\n" "${TOOLVERSION}" "${MINVERSION}" | sort -rV | head -n 1) | ||
| test "${HIGHEST}" = "${TOOLVERSION}" && exit 0 | ||
|
|
||
| printf "\n%sversion %s is not >= to minimal required %s\n\n" "${TOOLSTR}" "${TOOLVERSION}" "${MINVERSION}" |
There was a problem hiding this comment.
maybe rephrase: printf "\nInvalid version, found %s%s, minimal required is %s\n\n" "${TOOLSTR}" "${TOOLVERSION}" "${MINVERSION}"
The >= should be implied by minimal required.
There was a problem hiding this comment.
Will do, I should also print to stderr.
cladmi
changed the title
cladmi
force-pushed
the
pr/make/ccn-lite-relay/cmake_version
branch
from
5e1703f to
08f2ab0
Compare
|
Updated. Tell me if you want an output that I silent in make. |
smlng
added
Type: enhancement
|
Can I squash ? |
|
yes please! |
usage: has_minimal_version.sh <version> <minimal_version> [toolname]
Checks that version >= minimal_version
Version format MAJOR.MINOR.PATCH ex 3.1.4
ccn-lite does not build with cmake < 3.6.0. This checks for a minimal version
instead of failing to compile.
cmake version 3.5.2 is not >= to minimal required 3.6.0
Makefile:25: recipe for target '..cmake_version_supported' failed
Tested with versions 3.5.2, 3.6.0-rc1, 3.6.0, 3.6.3, 3.7.0-rc1, 3.7.0.
Note: the check used does not consider '-rcX' as 'sort -V' does not handle them
properly.
relic does not build with cmake < 3.6.0. This checks for a minimal version
instead of failing to compile.
cmake version 3.5.2 is not >= to minimal required 3.6.0
Makefile:25: recipe for target '..cmake_version_supported' failed
cladmi
force-pushed
the
pr/make/ccn-lite-relay/cmake_version
branch
from
08f2ab0 to
1aebe28
Compare
|
The rebase has the same diff as before, except that ccn-lite version was bumped in between. I retested compiling |
4 participants
Contribution description
ccn-lite/relic do not build with cmake < 3.6.0. This checks for a minimal version
instead of failing to compile.
Tested with versions 3.5.2, 3.6.0-rc1, 3.6.0, 3.6.3, 3.7.0-rc1, 3.7.0.
Note: the check used does not consider '-rcX' as 'sort -V' does not handle them
properly.
Testing
You can test this by compiling
examples/ccn-lite-relaywith different cmake versions:Last 3.5 release
First 3.6 stable release
You can download cmake old versions here:
https://cmake.org/files/
Issues/PRs references
Fixes #9064 by making the problem clear
Found during release testing.