[riot-notifications] [RIOT-OS/RIOT] murdock: allow multiple files to be sent along with a test job (#11697)

Gaëtan Harter notifications at github.com
Tue Jul 9 18:49:17 CEST 2019


cladmi commented on this pull request.

Is it normal to generate files outside of `BINDIR` or `BINDIRBASE`?
I thought we were trying to get rid of them.

> But if now a local user tries "make test-murdock", with the keys residing outside of BINDIR, it fails because of that.

I would not support any direction where a *private key* needs to be sent to an external test setup. These things are normally stored in a central system place with limited permissions and a passphrase or even on external hardware to never be seen.

I mean, why does the test script need to even use the private key?
The update mechanism is supposed to work with signed files.
It is the goal that the files can be signed files can be freely distributed without ever disclosing the private key.

Does it mean compilation or generation on the test device? This implies that generation tools are put as input to `test-input-hash` otherwise it is just inconsistent.

> -    [ ! -f "$flashfile" ] && {
-        echo "$0: _test(): flashfile \"$flashfile\" doesn't exist!"
-        return 1
-    }
+    # interpret any extra arguments as file names.
+    # They will be sent along with the job to the test worker
+    # and stored in the application's binary folder.
+    shift 2
+    local files=""
+    for filename in "$@"; do
+        # check if the file is within $(BINDIR)
+        if startswith "${BINDIR}" "${filename}"; then
+          # get relative (to $(BINDIR) path
+          local relpath="$(realpath --relative-to ${BINDIR} ${filename})"
+        else
+          local relpath="$(basename ${filename})"

If a file was in a subdirectory it ends up at the root of bindir. So not in the same place as the makefile would remotely expect it.

I do not see how this would work without a specific `murdock` hack in the test.

It should just be required or handled.

>  
-    [ ! -f "$flashfile" ] && {
-        echo "$0: _test(): flashfile \"$flashfile\" doesn't exist!"
-        return 1
-    }
+    # interpret any extra arguments as file names.
+    # They will be sent along with the job to the test worker
+    # and stored in the application's binary folder.
+    shift 2
+    local files=""
+    for filename in "$@"; do
+        # check if the file is within $(BINDIR)
+        if startswith "${BINDIR}" "${filename}"; then
+          # get relative (to $(BINDIR) path
+          local relpath="$(realpath --relative-to ${BINDIR} ${filename})"

* `realpath` is not supported on `OSx` https://github.com/RIOT-OS/RIOT/pull/10568#issuecomment-445221113 so it avoided in the build system. If we can force them to install it I would gladly use it at some other places.
* `BINDIR` is allowed to be locally overwritten and it is not handled.

If even supported to not be in `BINDIR`, I would handle it relative to `RIOTBASE` and only in a subdirectory of `RIOTBASE`.
Relative previous directories handling is the first thing banned in any services to keep sandboxed.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11697#pullrequestreview-259609125
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190709/7fd4993c/attachment-0001.html>


More information about the notifications mailing list