Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ParthShitole
Copy link

  • The added code checks for required dependencies before adding tests.
  • This makes sures the Test don't fail due to missing dependencies.

solves #4535

@fruffy fruffy added the ebpf Topics related to the eBPF back end label Oct 22, 2024
@ParthShitole
Copy link
Author

ParthShitole commented Oct 22, 2024

@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.
I request you to review it once. if it looks good to you, I will add these changes for other backends as well.

backends/ebpf/CMakeLists.txt Show resolved Hide resolved
backends/ebpf/CMakeLists.txt Outdated Show resolved Hide resolved
backends/ebpf/CMakeLists.txt Outdated Show resolved Hide resolved
@ParthShitole ParthShitole force-pushed the backend-tests branch 5 times, most recently from 6149a81 to 799c333 Compare October 23, 2024 09:57
@fruffy
Copy link
Collaborator

fruffy commented Oct 23, 2024

The MacOS failure is unrelated and fixed by #4976.

Copy link
Collaborator

@fruffy fruffy left a 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.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
backends/ebpf/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
backends/ebpf/CMakeLists.txt Outdated Show resolved Hide resolved
@fruffy fruffy added the infrastructure Topics related to code style and build and test infrastructure. label Oct 23, 2024
@fruffy
Copy link
Collaborator

fruffy commented Oct 23, 2024

You also need to sign DCO, please see instructions here: https://github.com/p4lang/p4c/blob/c05b04c3785059b1bf4faa2354377f0f47fb7950/CONTRIBUTING.md#contributing-license

ParthShitole and others added 3 commits October 24, 2024 11:14
…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]>
@ParthShitole ParthShitole force-pushed the backend-tests branch 2 times, most recently from a5c9015 to 90939af Compare October 24, 2024 05:48
ParthShitole and others added 2 commits October 24, 2024 11:20
- 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]>
Copy link
Collaborator

@fruffy fruffy left a 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 ?

Copy link
Collaborator

@fruffy fruffy left a 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.

backends/ebpf/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/P4CUtils.cmake Outdated Show resolved Hide resolved
@ParthShitole ParthShitole force-pushed the backend-tests branch 3 times, most recently from e7d46a0 to 24e7de4 Compare October 27, 2024 07:03
- 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]>
- Corrected the specific dependencies to be checked before adding tests for ebpf

Signed-off-by: Parth Shitole <[email protected]>
@ParthShitole
Copy link
Author

@fruffy I have made the requested changes. Can you review it again.

@fruffy
Copy link
Collaborator

fruffy commented Nov 19, 2024

@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)
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ebpf Topics related to the eBPF back end infrastructure Topics related to code style and build and test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants