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

Make eBPF CMAKE robust against llvm check failures #1946

Merged
merged 2 commits into from
May 23, 2019

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented May 23, 2019

This pull request is intended to introduce a workaround for #1376.
Instead of using CMAKE's find_package we manually check the LLVM version and compare. This prevents crashes with specific versions of LLVM and Ubuntu.
The downside of this method is that it does not catch versions of LLVM that were installed (via apt install llvm-3.8 for example) and disables the eBPF backend despite an LLVM version that exists.
However, because the eBPF framework uses llc anyway instead of, say, llc-3.8 it is probably for the better that this discrepancy is caught.

Let me know what you think.

Ignoring ebpf tests...")
set (SUPPORTS_KERNEL False)
endif()
else()
message(STATUS "LLVM missing, disabling kernel tests...")
message(WARNING
"Problem with LLVM, disabling kernel tests...\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message could be more user-friendly.
Perhaps "Could not detect a version of LLVM which is needed to compile the eBPF kernel tests".

Did you test this on both systems that have working and broken llvms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I tested it on an Ubuntu Xenial image with the broken llvm package and on my local version. Both worked fine. The Travis test also passed. This method does not seem to work on macOS, but we do not support that anyway as far as I remember.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mac we don't support eBPF, but the build should still work, of course.
Travis will test on Mac, so I think that if that passes (and it did previously) then we can merge this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it should pass, it will just complain that it cannot find llvm, which also might be just a setup issue in the macOS Travis image.

@mihaibudiu mihaibudiu merged commit b806cec into p4lang:master May 23, 2019
@fruffy fruffy deleted the patch-1 branch June 20, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants