murdock: allow multiple files to be sent along with a test job

[kaspar030]

Contribution description

Previously, this was hard-coded to allow one file, hard-coded to be
called "flash file".
This commit allows multiple files to be specified via adding them to the
TEST_EXTRA_FILES variable. All files will be stored in the worker's
application bin directory.

Also, the existence check has been removed, as dwqc bails out on missing
file anyways.

Testing procedure

confirm CI tests on hardware are still working.

Issues/PRs references

Prerequisite for #11707.

[cladmi]

No "issue/PRs References", how is this supposed to be used ?

The testing procedure only shows that it does not break the previous state not that it solved a new one.

From usage it looks like the files in TEST_EXTRA_FILES must be in BINDIR, would be nice to be documented.

[kaspar030]

No "issue/PRs References", how is this supposed to be used ?

A PR using the new functionality is now open. I've added the reference.

The testing procedure only shows that it does not break the previous state not that it solved a new one.

Please use #11707 as test for the new functionality.

From usage it looks like the files in TEST_EXTRA_FILES must be in BINDIR, would be nice to be documented.

Hm, how did you deduce that? Any file from any path can be specified, but they will end up in BINDIR.

[cladmi]

From usage it looks like the files in TEST_EXTRA_FILES must be in BINDIR, would be nice to be documented.

Hm, how did you deduce that? Any file from any path can be specified, but they will end up in BINDIR.

That is why I need a use case.

From this PR only, as the test script is run without additional configuration, there is no information of where the file will be for the test script, so I imagine this will be set in the build system and so should be in the "same" place.

[kaspar030]

That is why I need a use case.

Do you need more use-case than #11707?

[cladmi]

That is why I need a use case.

Do you need more use-case than #11707?

I should have written "needed" here. My bad.

[cladmi]

From the use case, I would not add a murdock specific variable.
The files will also need to also be added to link:.

It would make more sense from a general build system point of view to have a COMPILED_BINARIES, DISTRIBUTED_FILES or another name that by default contains $(ELFFILE) $(FLASHFILE) (and currently $(HEXFILE)).
And that this list of files must be in directory/subdirectory of BINDIR.

It was in my plans, but was for after the FLASHFILE migration.

I think that currently murdock does not handle testing applications with mcuboot and nordic_softdevice either.
And there, the mcuboot.bin and softdevice.hex would not be TEST_FILES but generally needed files (they just need to have a file target too).

As a result, it would by default also upload the ELFFILE to the test environment, but makes sense if we need to do debugging.

[kaspar030]

From the use case, I would not add a murdock specific variable.

Well, as is this is not murdock specific. There's just no other use-case (for now) that splits compilation and test execution.

The files will also need to also be added to link:.

#11707 adds the riotboot files to "link".

It would make more sense from a general build system point of view to have a COMPILED_BINARIES, DISTRIBUTED_FILES or another name that by default contains $(ELFFILE) $(FLASHFILE) (and currently $(HEXFILE)).

Well, right now the use case is testing. I'm happy to rename the variable to whatever fits better. Should do that when there is another use-case.

I did not fold ELFFILE. FLASHFILE etc. together into one variable so it is more flexible. Sending large .elf files does matter for the CI use case. Currently, only one test will make use of this facility. This should not add cost to others.

And that this list of files must be in directory/subdirectory of BINDIR.

IMO that is a useless restriction. In its current implementation, only the target is restricted, but more for lazyness of getting the relative handling path right wrt. differing BINDIRs on build host and test host. This is very fine for the intended use case. We should get that in and improve later.

[cladmi]

And that this list of files must be in directory/subdirectory of BINDIR.

IMO that is a useless restriction. In its current implementation, only the target is restricted, but more for lazyness of getting the relative handling path right wrt. differing BINDIRs on build host and test host. This is very fine for the intended use case. We should get that in and improve later.

Without "documenting" the TEST_EXTRA_FILES must be generated files at the base of bindir or the current implementation is inconsistent.

The murdock case needs a test target that has the exact same riot version on it.
So the test will expect the file in the "same" location on the test target, as it is running the same make test. So it would not use a file that has been moved to a new location. Introducing different behavior on the test target is not something I would support.

If other files than ones in bindir must be considered for the test-input-hash, like some other sources files, then they should not be uploaded to bindir. The current implementation uploads everything, so I assume the files in TEST_EXTRA_FILES must be in BINDIR.

I do not see any reason to upload other generated files than files from bindir as we are not supposed to generate files in the source tree. An exception I see could be another application bin directory in the case of the bootloader for example. However, I would rather copy the file in the tested application bindir instead of guessing the bootloader application bindirectory. (Hmm just makes me think that currently, for riotboot, the bootloader is not compiled in BINDIR).

For uploads, I would only handle relative path inside BINDIR. Having ../ relative path is opening to "lets get files outside of the repository I should not be reading" kind of issues.

The files will also need to also be added to link:.

#11707 adds the riotboot files to "link".

They must only be generated in the building machine, so not on the host when building in docker. This should also be documented.

[cladmi]

I noticed one bug I think, we are checking the ELFFILE hash and uploading the FLASHFILE.

While thinking about it a bit more, also applied to the given example, I imagine somehow two variables for this.

• APPLICATION_BIN_FILES or something that described the compiled files that are used. For the normal usage, FLASHFILE and should also contain softdevice.hex I imagine. And these files should course be uploaded to bindir. Putting ELFFILE there would be specific to the test if we want one running debug. These ones could be automatically added as dependency to link in Makefile.include.
• TESTS_INPUT_FILES for files that should be taken into account when checking the test hash which by default would have TESTS_INPUT_FILES += $(TESTS) $(APPLICATIONS_TEST_BIN_FILES)
  And could also contain other, non generated files that are part of the repository and need to be taken into account. Currently, the usage PR is using other files not in TESTS as test inputs.

[kaspar030]

I changed TEST_EXTRA_FILES to only allow files from $(BINDIR), and to keep the relative path from there.

I noticed one bug I think, we are checking the ELFFILE hash and uploading the FLASHFILE.

Could change to check FLASHFILE (now that we have one). Other construction zone, though.

While thinking about it a bit more, also applied to the given example, I imagine somehow two variables for this.

Let's not overthink this. Usually, FLASHFILE is all that is needed. In the RIOTBOOT tests case, some extra files are needed. softdevice will be gone before we can add it here.

[kaspar030]

I changed TEST_EXTRA_FILES to only allow files from $(BINDIR), and to keep the relative path from there.

This was not sufficient for transmitting the suit update keys.
The current key scheme (in a WIP branch) is as follows:

1. the keys are supposed to be in SUIT_KEY_DIR
2. that defaults to $(RIOTBASE)/keys, so if a user recompiles (e.g., with "make clean all"), the keys are kept
3. if RIOT_CI_BUILD is set, SUIT_KEY_DIR defaults to $(BINDIR)

So far so good. For a regular CI build, the keys from BINDIR are sent along with the test job.
But if now a local user tries "make test-murdock", with the keys residing outside of BINDIR, it fails because of that.

So I changed the logic that if a file is within BINDIR, it'll put into the path relative to BINDIR on the worker node. If a file is not in BINDIR, it's to BINDIR/$(basename $file).

[kaspar030]

@cladmi ping

[kaspar030]

ping

This is a dependency of #11707. It would be very handy to confirm riotboot support using an automated test, with upcoming release testing in mind.

[cladmi]

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.

[cladmi]

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.

[kaspar030]

This is "./,murdock". Everything in here is quite murdock specific. Yes, the SUIT makefile has a CI specific hack (if RIOT_CI_BUILD: KEYDIR=BINDIR, else: ...), but that is a pragmatic solution that works in practice.

Where exactly do you see an issue?

[cladmi]

basename.

[cladmi]

• realpath is not supported on OSx makefiles/docker: fix mounting localtime on OSX #10568 (comment) 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.

[kaspar030]

* `realpath` is not supported on `OSx` [#10568 (comment)](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.

All CI workers are required to provide realpath. This is a closed ecosystem, if we (finally) add OSX builders, we can surely make them also provide that.

* `BINDIR` is allowed to be locally overwritten and it is not handled.

Local BINDIR overrides are handled by using BINDIR as variable. Remote (on workers) BINDIR overrides are currently not handled, as that is currently not needed. There's a comment on that.

Relative previous directories handling is the first thing banned in any services to keep sandboxed.

A check whether one of the files is are in RIOTBASE can be easily added.

[kaspar030]

A check whether one of the files is are in RIOTBASE can be easily added.

they now have to be in BINDIR.

[kaspar030]

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.

This is meant for testing, the key does not need to be kept secret. Providing a secure key distribution system in order to provide a rather open test system with "secure" keys is a complete waste of time.

[kaspar030]

I mean, why does the test script need to even use the private key?
The update mechanism is supposed to work with signed files.

The update mechanismn works with signed files.

Having the key available on the test workers makes the test much more flexible. Without the key, tests like "try updating a lower version number, see it fail" or "provide a correctly signed image with broken sha checksum" etc. would need all of the necessary signed binaries to be created at compile time, on the compilation worker. That would split the testing logic between the test script and the test application Makefile, instead of having it contained in just the test script.

[fjmolinas]

It would make more sense from a general build system point of view to have a COMPILED_BINARIES, DISTRIBUTED_FILES or another name that by default contains $(ELFFILE) $(FLASHFILE) (and currently $(HEXFILE)).

Well, right now the use case is testing. I'm happy to rename the variable to whatever fits better. Should do that when there is another use-case.

Although I think @cladmi comment here is valid I agree with the fact that this can be added when the use case arises, no need to add it now when there is no use-case.

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.

This is meant for testing, the key does not need to be kept secret. Providing a secure key distribution system in order to provide a rather open test system with "secure" keys is a complete waste of time.

I agree witch @cladmi concern here, even if I'm testing I do not think my personal private keys should be sent.

But I don't see why a pair of keys can't be generated just for testing, and those be sent out to the workers? Maybe I'm not following correctly but isn't that what is happening here? make tests-murdock sets RIOT_CI_BUILD, SUIT_KEY_DIR is forced to $(BINDIR), a temporary just for testing pair of keys is generated, the $(FLASH_FILE) would be recompiled before sending and everything in murdock would run with this temporary pair of keys?

So far so good. For a regular CI build, the keys from BINDIR are sent along with the test job.
But if now a local user tries "make test-murdock", with the keys residing outside of BINDIR, it fails because of that.

If the user insists on using his personal private keys for testing a warning message where he is informed that they are being sent in a insecure way should be enough for now, or even an error message which the user could comment out to force it. What do you think?

[fjmolinas]

The files will also need to also be added to link:.

#11707 adds the riotboot files to "link".

They must only be generated in the building machine, so not on the host when building in docker. This should also be documented.

I think this comment is still not addressed, right?

[kaspar030]

If the user insists on using his personal private keys for testing a warning message where he is informed that they are being sent in a insecure way should be enough for now, or even an error message which the user could comment out to force it. What do you think?

Well, I've removed the support for "TEST_EXTRA_FILES" outside of BINDIR.
For CI, nothing changes. The user now has to do SUIT_KEY_DIR='$(BINDIR)' make all test-murdock. If the user insists of using non-generated keys, they have to manually copy the key files into BINDIR.

That alright with everyone?

[fjmolinas]

That alright with everyone?

Im ok with this.

[kaspar030]

I think this comment is still not addressed, right?

IIRC this is not part of this PR.

[fjmolinas]

@kaspar030 sorry for taking sor long, I didn't realize @cladmi basename comment had been addressed. It all looks good to me, can you squash? Lets see CI:run tests output.

[kaspar030]

I've disabled the test cache so all tests get run.

[fjmolinas]

The CI shows no regression as far as I can tell, I see that the review concerns have been addressed and I see a clear use case for #11707 and future tests. ACK and GO!