Page MenuHomePhabricator

Fix OCaml build failure because of absolute path in system libs
ClosedPublic

Authored by haampie on Aug 18 2020, 4:37 AM.

Details

Summary

https://reviews.llvm.org/D85820 introduced a full path in the LLVM_SYSTEM_LIBS property of the LLVMSupport target, which made the OCaml bindings fail to build, since they use -l [system_lib] flags for every lib in LLVM_SYSTEM_LIBS, which cannot work with absolute paths.

This patch solves the issue in a similar vain as ZLIB does it: it adds the full library path to imported_libs, and adds a stripped down version without directories, lib prefix and lib suffix to system_libs

In the future we should probably make some changes to LLVM_SYSTEM_LIBS, since both zlib and ncurses do not necessarily have to be system libs anymore due to the find_package / find_library bits introduced in https://reviews.llvm.org/D85820 and https://reviews.llvm.org/D79219.

Diff Detail

Event Timeline

haampie created this revision.Aug 18 2020, 4:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2020, 4:37 AM
haampie requested review of this revision.Aug 18 2020, 4:37 AM
haampie added a reviewer: RKSimon.
haampie edited the summary of this revision. (Show Details)Aug 18 2020, 4:40 AM
dmajor added a subscriber: dmajor.Aug 18 2020, 8:19 AM
phosek added inline comments.Aug 18 2020, 12:52 PM
llvm/lib/Support/CMakeLists.txt
5

We typically use lowercase for function arguments, also the output variable usually goes at the end (unless the function has variable number of arguments).

haampie updated this revision to Diff 286413.Aug 18 2020, 3:53 PM

Move the output variable to the end and use lowercase for arguments.

Change the library name regex to use CMAKE_FIND_LIBRARY_PREFIXES and CMAKE_FIND_LIBRARY_SUFFIXES, since those are used by find_library. This fixes an issue on macOS for zlib introduced in https://reviews.llvm.org/D79219 too, since find_package finds a stub library with a .tbd suffix on darwin by default.

Before this differential on macOS (from BuildVariables.inc):

#define LLVM_SYSTEM_LIBS "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/lib/libcurses.tbd -lm -llibz.tbd -lxml2"

After:

#define LLVM_SYSTEM_LIBS "-lm -lz -lcurses -lxml2"
haampie marked an inline comment as done.Aug 18 2020, 3:54 PM
phosek accepted this revision.Aug 18 2020, 3:55 PM

LGTM

This revision is now accepted and ready to land.Aug 18 2020, 3:55 PM

@haampie Do we have a confirmation that this works yet? I'm willing to do a test commit before too much of the US wakes up.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

CMake Error at lib/Support/CMakeLists.txt:223 (get_system_libname):
  get_system_libname Function invoked with incorrect arguments for function
  named: get_system_libname

The failing build is http://lab.llvm.org:8011/builders/sanitizer-windows/builds/68331

I'm going to revert the change, but I'm happy to reland it once we figure out what the problem is.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

CMake Error at lib/Support/CMakeLists.txt:223 (get_system_libname):
  get_system_libname Function invoked with incorrect arguments for function
  named: get_system_libname

The failing build is http://lab.llvm.org:8011/builders/sanitizer-windows/builds/68331

I'm going to revert the change, but I'm happy to reland it once we figure out what the problem is.

I also think this broke the Windows builds for Chrome: https://crbug.com/1119478.
CMake Error at lib/Support/CMakeLists.txt:9 (STRING):

STRING sub-command REGEX, mode REPLACE: regex "^()" matched an empty
string.

Call Stack (most recent call first):

lib/Support/CMakeLists.txt:218 (get_system_libname)

Might need zlib to repro.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

CMake Error at lib/Support/CMakeLists.txt:223 (get_system_libname):
  get_system_libname Function invoked with incorrect arguments for function
  named: get_system_libname

The failing build is http://lab.llvm.org:8011/builders/sanitizer-windows/builds/68331

I'm going to revert the change, but I'm happy to reland it once we figure out what the problem is.

I also think this broke the Windows builds for Chrome: https://crbug.com/1119478.
CMake Error at lib/Support/CMakeLists.txt:9 (STRING):

STRING sub-command REGEX, mode REPLACE: regex "^()" matched an empty
string.

Call Stack (most recent call first):

lib/Support/CMakeLists.txt:218 (get_system_libname)

Might need zlib to repro.

I've addressed the terminfo issue with D86234, I can look into the zlib issue.

llvm/lib/Support/CMakeLists.txt
222

We might need to gate this block on CMAKE_HOST_UNIX just like we do above, but I'm also a bit surprised that LLVM_ENABLE_TERMINFO is TRUE on Windows?

@aeubanks and @phosek: see https://reviews.llvm.org/D86245 for a last fix that checks whether CMAKE_FIND_LIBRARY_PREFIXES and CMAKE_FIND_LIBRARY_SUFFIXES are non-empty before applying the regex to fix this window issue