make/pkg: allow to set SOURCE_LOCAL per pkg #11533
Conversation
pkg/pkg.mk
Outdated
| @@ -3,6 +3,7 @@ | |||
| # | |||
| PKG_DIR?=$(CURDIR) | |||
| PKG_BUILDDIR?=$(PKGDIRBASE)/$(PKG_NAME) | |||
| PKG_SOURCE_LOCAL ?= $(PKG_$(shell echo $(PKG_NAME) | tr a-z A-Z)_SOURCE_LOCAL) | |||
There was a problem hiding this comment.
I'd prefer PKG_SOURCE_LOCAL ?= $(PKG_SOURCE_LOCAL.$(PKG_NAME)) to be more in line with the proposal in #10928.
It also removes the extra subshell call just to uppercase the package name
That would also allow sth like foreach pkg in $USEPKG) do export $(PKG_SOURCE_LOCAL.$(PKG_NAME)).
Unfortunately shell doesn't allow dots in variable names, so when overriding from the command line, this would have to be specified as make argument, e.g.:
# works:
make PKG_SOURCE_LOCAL.heatshrink=~/src/heatshrink
# doesn't work:
PKG_SOURCE_LOCAL.heatshrink=~/src/heatshrink make
# beware: doesn't work either:
PKG_SOURCE_LOCAL_CN-CBOR=foo make
There was a problem hiding this comment.
Hm, the command line issue is pretty unfortunate, especially as I personally prefer to use exactly the case that would not be supported anymore :-(
But keeping it consistent makes sense. Could using a _ instead of . make, also in #10928, make sense maybe? In terms of parsing, it wouldn't make a difference. And also for human readability, SOURCE_LOCAL_nimble vs SOURCE_LOCAL.nimble is not the worst in the world, considering the benefits of the first option. Also USEMODULE_INCLUDES_posix vs USEMODULE_INCLUDES.posix could IMHO work, right?
There was a problem hiding this comment.
In terms of parsing, it wouldn't make a difference.
Really? Because it is easier to find a completely different symbol as delimiter than a character that can already appear in both variable name and module name.
There was a problem hiding this comment.
I think the readability is pretty much subjective. But IMHO the key is the usability here: I would in this case surely trade (subjective) readability for versatility. Wouldn't you agree?
There was a problem hiding this comment.
I don't understand, how SOME_VAR.vars_scope is less readable than SOME_VAR_vars_scope, but as you said.. it's subjective.
There was a problem hiding this comment.
But IMHO the key is the usability here: I would in this case surely trade (subjective) readability for versatility. Wouldn't you agree?
And no, if something is objectively more versatile but only subjectively more readable I don't agree with this trade.
There was a problem hiding this comment.
clarified offline: we both agree that usefulness is more important the readability :-)
There was a problem hiding this comment.
Yepp, and regarding compatibility of #10928: this is a very simple one-line change and #10928 was not further discussed since February. So I say: let's merge it, as it is a very useful change (for package development and in cases where you don't have Internet (e.g. on a train)). However, the point @kaspar030 made about - still upholds, so please provide a pattern replacement for that as well.
45cb306 to
195f1fb
Compare
|
pushed a change/fix:
I'd say the goal of aligning this approach with #10928 still holds and is not solved by this PR. But for now, I'd like to go with the PR as is and adapt it later if needed, as it would make my life as a developer working with packages on a daily base a lot easier... For completeness, here is a major use-case that I forgot to mention: exporting the variable for a shell session is something that should work. Unfortunately that is another thing that won't accept |
There was a problem hiding this comment.
ACK, tested with tests/lwip and examples/ccn-lite-relay
$ PKG_SOURCE_LOCAL_LWIP=${PWD}/../lwip make -C tests/lwip
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/lwip'
Building application "tests_lwip" for "native" with MCU "native".
rm -Rf /home/mlenders/Repositories/RIOT-OS/RIOT/tests/lwip/bin/pkg/native/lwip
mkdir -p $(dirname /home/mlenders/Repositories/RIOT-OS/RIOT/tests/lwip/bin/pkg/native/lwip)
cp -a /home/mlenders/Repositories/RIOT-OS/RIOT/../lwip /home/mlenders/Repositories/RIOT-OS/RIOT/tests/lwip/bin/pkg/native/lwip
[…]
$ PKG_SOURCE_LOCAL_CCN_LITE=${PWD}/../ccn-lite/ make -C examples/ccn-lite-relay/
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/examples/ccn-lite-relay'
Building application "ccn-lite-relay" for "native" with MCU "native".
rm -Rf /home/mlenders/Repositories/RIOT-OS/RIOT/examples/ccn-lite-relay/bin/pkg/native/ccn-lite
mkdir -p $(dirname /home/mlenders/Repositories/RIOT-OS/RIOT/examples/ccn-lite-relay/bin/pkg/native/ccn-lite)
cp -a /home/mlenders/Repositories/RIOT-OS/RIOT/../ccn-lite/ /home/mlenders/Repositories/RIOT-OS/RIOT/examples/ccn-lite-relay/bin/pkg/native/ccn-lite
[…]
|
Ping @kaspar030. Your comment was addressed |
|
@haukepetersen please squash, this is very useful and such a small change. |
|
@haukepetersen please squash! |
195f1fb to
cf8f312
Compare
|
squashed |
|
thanks for approving, this will make my life (and hopefully others) much easier :-) |
Contribution description
When working with packages in RIOT, the
PKG_SOURCE_LOCALvariable is extremely helpful. But it currently needs to be set in the package Makefile, hence introducing 'debug' code into the RIOT source tree.This PR allows to set this variable globally on a per-pkg basis, by simply mapping the package name in a variable name, e.g. building with
PKG_MYPKG_SOURCE_LOCAL=/some/dir make ...will build an application withPKG_SOURCE_LOCAL=/some/dirset for the packageRIOT/pkg/mypkg.Only caveat so far: if set in the applications Makefile, it needs to be exported.
Testing procedure
Take a package and a fitting RIOT application of your choice and set the according
PKG_xxx_SOURCE_LOCALvariable to the local source folder of that package. You should see in the build output, that the package is actually copied from the specified location to the bin folder, instead of being checked out from the configured repository.Issues/PRs references
none