-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent compiling backend tests if dependencies are not installed #4973
base: main
Are you sure you want to change the base?
Conversation
@fruffy I have modified the CMakeLists.txt file for ebpf backend to check for dependencies. If the dependencies are missing the tests are not added and appropriate message is printed. |
6149a81
to
799c333
Compare
The MacOS failure is unrelated and fixed by #4976. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Some comments.
You also need to sign DCO, please see instructions here: https://github.com/p4lang/p4c/blob/c05b04c3785059b1bf4faa2354377f0f47fb7950/CONTRIBUTING.md#contributing-license |
7c44e03
to
fdb8954
Compare
…t installed - The added code checks for required dependencies before adding tests. - This makes sures the Test don't fail due to missing dependencies. Signed-off-by: Parth Shitole <[email protected]>
- The Previous implementation of a macro is now converted to a function and placed in the P4CUtils.cmake file - Other minor changes are made to improve the functionality. Signed-off-by: Parth Shitole <[email protected]>
Removing Unintended changes Signed-off-by: Parth Shitole <[email protected]>
a5c9015
to
90939af
Compare
- Intially the HINTS variable for find_libraries was hard coded. - The path may change for different Operating Systems. - The HINTS parameter can now be populated by the caller, and will only be used if provided. - Minor Typos are also fixed. Signed-off-by: Parth Shitole <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the work!
Before approving, I will need to try this locally to see whether it correctly detects missing dependencies. Any other comments @ChrisDodd ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, now that I had some time to try this out some more detailed feedback.
e7d46a0
to
24e7de4
Compare
- The current implementation of the above function may suffer from stallness. - Required changes are made to avoid this. - Some minor typos are also fixed. Signed-off-by: ParthShitole <[email protected]>
24e7de4
to
43252ef
Compare
- Corrected the specific dependencies to be checked before adding tests for ebpf Signed-off-by: Parth Shitole <[email protected]>
@fruffy I have made the requested changes. Can you review it again. |
Let me take a look later this week. |
@@ -222,10 +222,10 @@ else() | |||
endif() | |||
|
|||
# List of executable dependencies | |||
set(TEST_DEPENDENCY_PROGRAMS tcpdump) | |||
set(TEST_DEPENDENCY_PROGRAMS gcc-multilib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked whether this actually works? I think you need to do a different check to make sure this is installed.
solves #4535