Sanitize file path input in cmake, add travis check to skip tests #1476
Conversation
| set (__xfail_list "${xfail}") | ||
| set (__test_list "${tests}") | ||
| # Sanitize the input lists to account for mixed path input | ||
| p4c_set_abs_path("${xfail}" __xfail_list) |
There was a problem hiding this comment.
@cc10512 : can you please take a look at the changes in this file?
I don't know of all the uses of the build process to correctly review this file.
There was a problem hiding this comment.
I am also happy if we find a better way to make sure that all paths have the same root folder. Right now, I am casting to an absolute path and then back to a relative one. That does the job, but it is not great.
Oh and in p4c_add_tests only the "testsuites" list is checked with p4c_find_test_names, "xfail" is passed along as is. I was sure if that is intentional.
There was a problem hiding this comment.
As you correctly state, each element in the "tests" list is relative to P4C_SOURCE_DIR. The xfails are based on whatever you write as xfail in your CMakeLists. We typically write them based on P4C_SOURCE_DIR as well. For example: 'testdata/p4_16_samples/issue774-4-bmv2.p4' or 'extensions/<extension_name>/p4_16/ingress_checksum.p4'.
I suppose you could normalize them all to an absolute path. However, since you're converting back to relative paths, why not simply convert the xfails to relative paths based on P4C_SOURCE_DIR and leave the tests as specified?
There was a problem hiding this comment.
I think I remember the problem I was dealing with. If the input is already a relative path, which is often the case, the cmake function file RELATIVE PATH throws an error:
file RELATIVE_PATH must be passed a full path to the file.
Which is why I first converted all inputs to a full path. If I modify p4c_find_test_names to use the absolute path instead of a relative one, no test gets added at all. Likely because of some specific behaviour in p4c_add_test?
There was a problem hiding this comment.
Likely. We do name the tests based on the path unless you provide an alias, and in that case cmake may barf. I don't recall, but if you attach a log, I could take a look. However, I do think that if you just convert xfails to absolute paths and then back to relative w.r.t. P4C_SOURCE_DIR, you should be in good shape. That is, you don't need the absolute path comparison, you just seem to need to converge on one convention. The one we've been following is to provide the correct relative path for elements in the xfail list. Moreover, if you have multiple tests with the same name in different directories, you'll have to provide enough of their path in the xfail spec to distinguish. So, overall, I think you're better off giving the full relative path!
| @@ -145,3 +146,9 @@ def check_root(): | |||
| """ This function returns False if the user does not have root privileges. | |||
| Caution: Only works on Unix systems """ | |||
| return (os.getuid() == 0) | |||
|
|
|||
|
|
|||
| def check_travis(): | |||
There was a problem hiding this comment.
You could run the docker container in privileged mode in Travis: docker run --privileged ... and then you can have your cake and run all your kernel tests.
There was a problem hiding this comment.
Hmm, I tried this here, but unfortunately the problem persists. I do not know about the cause. Something is preventing the build inside the Travis environment from creating new namespaces.
| set (__xfail_list "${xfail}") | ||
| set (__test_list "${tests}") | ||
| # Sanitize the input lists to account for mixed path input | ||
| p4c_set_abs_path("${xfail}" __xfail_list) |
There was a problem hiding this comment.
As you correctly state, each element in the "tests" list is relative to P4C_SOURCE_DIR. The xfails are based on whatever you write as xfail in your CMakeLists. We typically write them based on P4C_SOURCE_DIR as well. For example: 'testdata/p4_16_samples/issue774-4-bmv2.p4' or 'extensions/<extension_name>/p4_16/ingress_checksum.p4'.
I suppose you could normalize them all to an absolute path. However, since you're converting back to relative paths, why not simply convert the xfails to relative paths based on P4C_SOURCE_DIR and leave the tests as specified?
|
I will merge this to fix the breakage of the ebpf tests; if there are further changes please submit another PR. |
This requests modifies
p4c_add_test_listto add the absolute path to file inputs. This is necessary for xfail tests in extensions such as p4c-xdp, which did not properly matched and were ignored.Another addition is a check for Travis builds in the ebpf-kernel tests. It seems that due to privilege issues Travis does not support netem or ip netns commands:
mount --make-shared /var/run/netns failed: Operation not permittedThe check is to ensure that run does not get executed. Ideally, we still want to check that loading is possible, but this requires creating a new type of virtual environment.