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

Fix issue LLVM linking with libclamav_rust test executable #572

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Fix issue LLVM linking with libclamav_rust test executable #572

merged 1 commit into from
Oct 25, 2022

Conversation

teoberi
Copy link
Contributor

@teoberi teoberi commented May 6, 2022

Fix for issue "Clamav 0.105.0 test libclamav_rust failed #569"

@micahsnyder micahsnyder changed the title Update CMakeLists.txt Fix issue LLVM linking with libclamav_rust test executable May 6, 2022
@micahsnyder micahsnyder self-requested a review July 15, 2022 17:34
@micahsnyder micahsnyder added the 🍒cherry-pick-candidate A PR that should be backported once approved. label Jul 15, 2022
Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

This makes sense to me, and seems like a reasonable solution. I didn't see any adverse effects in testing.

@micahsnyder
Copy link
Contributor

micahsnyder commented Jul 18, 2022

Actually, I'm not entirely happy with the location where this is stripped. It is stripped before building a specific test executable, (check_fpu_endian), and just happens to propagate down to the libclamav_rust through the LLVM_LIBS environment variable. That makes it feel kind of hacky to me, and easy to accidentally break in the future.

It would be better to either do it:
A) just after LLVM_LIBRARIES is first defined in FindLLVM.cmake, here: https://github.com/Cisco-Talos/clamav/blob/main/cmake/FindLLVM.cmake#L154
or B) to the LLVM_LIBS variable just after LLVM_LIBS is created, here: https://github.com/Cisco-Talos/clamav/blob/main/unit_tests/CMakeLists.txt#L172

@teoberi would you please give one of these options a try to ensure it also resolves the issue and doesn't break anything else in your environment?

If you like, I can make the edit directly to your teoberi-fix-test-libclamav_rust branch.

@teoberi
Copy link
Contributor Author

teoberi commented Jul 19, 2022

I use Slackware64-current (development tree) on all my servers where it has already been switched to LLVM 14.0.6.
I no longer have any server with LLVM 13.0.1 and the build with LLVM 14.0.6 crashes at cmake --build . --config Release before I get to the ctest.
So in conclusion for Slackware I am stuck until problems related to building with LLVM 14 is solved.

@micahsnyder
Copy link
Contributor

Okay understood. I should've realized that based on our conversations in other issues. Okay I'll tweak it myself and test it as best I can.

@teoberi
Copy link
Contributor Author

teoberi commented Jul 20, 2022

I tested in a virtual machine with Slackware64-current and LLVM 13 the proposed solutions:
A) just after LLVM_LIBRARIES is first defined in FindLLVM.cmake -> it works

Test project /usr/local/src/clamav-0.105.0/build
Start 1: libclamav
1/6 Test 1: libclamav ........................ Passed 42.09 sec
Start 2: libclamav_rust
2/6 Test 2: libclamav_rust ................... Passed 0.38 sec
Start 3: clamscan
3/6 Test 3: clamscan ......................... Passed 10.93 sec
Start 4: clamd
4/6 Test 4: clamd ............................ Passed 29.23 sec
Start 5: freshclam
5/6 Test 5: freshclam ........................ Passed 4.18 sec
Start 6: sigtool
6/6 Test 6: sigtool .......................... Passed 1.00 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) = 87.83 sec

B) to the LLVM_LIBS variable just after LLVM_LIBS is created -> it works

Test project /usr/local/src/clamav-0.105.0/build
Start 1: libclamav
1/6 Test 1: libclamav ........................ Passed 43.09 sec
Start 2: libclamav_rust
2/6 Test 2: libclamav_rust ................... Passed 0.38 sec
Start 3: clamscan
3/6 Test 3: clamscan ......................... Passed 10.88 sec
Start 4: clamd
4/6 Test 4: clamd ............................ Passed 28.37 sec
Start 5: freshclam
5/6 Test 5: freshclam ........................ Passed 4.15 sec
Start 6: sigtool
6/6 Test 6: sigtool .......................... Passed 0.97 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) = 87.85 sec

I will test this again after solving the problems related to the build with LLVM 14 as a stage in the process of returning to Clamav.
Clamav was one of the first packages I builded in my early days in Linux I don't know how many years ago.

@micahsnyder
Copy link
Contributor

Awesome thanks @teoberi for testing.

When LLVM is found with FindLLVM.cmake module, it may set the
LLVM_LIBRARIES variable to have `-l` prefix on the libraries.
The Rust `build.rs` script does not like this and will end up trying to
link with `-l-lLLVM` instead of `-lLLVM`.

This commit strips the `-l` prefix from the LLVM_LIBRARIES variable
before passing it off to build the libclamav_rust test program.
@micahsnyder micahsnyder merged commit 7f3f2aa into Cisco-Talos:main Oct 25, 2022
@teoberi teoberi deleted the teoberi-fix-test-libclamav_rust branch October 25, 2022 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒cherry-pick-candidate A PR that should be backported once approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants