Page MenuHomePhabricator

Fix DynamicLibraryTests build on Windows when LLVM_EXPORT_SYMBOLS_FOR_PLUGINS is ON
ClosedPublic

Authored by michaelplatings on Nov 19 2018, 8:05 AM.

Details

Summary

extract_symbols.py (introduced in D18826) expects all of its library arguments to be in the same directory - typically <config>/lib. DynamicLibraryLib.lib is instead to be found in lib/<config>.
This patch intended to make DynamicLibraryLib.lib be created in <config>/lib alongside most of the other libraries.

I previously tried passing absolute paths to extract_symbols.py but this generated command lines that were too long for Visual Studio 2015: D54587

Diff Detail

Repository
rL LLVM

Event Timeline

I'm not a fan of writing test stuff into <buildroot>/lib.

Is this the only way the test will pass on Windows?
Is fixing extract_symbols.py to handle whatever is needed a better option?

Thanks for the review Frederich.

I'm not a fan of writing test stuff into <buildroot>/lib.

I hear you, but for single-configuration builds that's exactly where libDynamicLibraryLib.a is already, along with libgtest.a and libgtest_main.a. This change just makes the Windows build more consistent with builds on Linux etc.

Is fixing extract_symbols.py to handle whatever is needed a better option?

I don't believe it's a better option, but I could modify extract_symbols.py to additionally look in lib/<config>. I'll go with your decision.

I take it your using the MSVC generator?
I use ninja (which is a single-config build) all the time and it puts the the files into unittests/Support/DynamicLibrary ?
I'll try to spin up a Windows build in the next couple of days, but I'm not foreseeing my opinion being changed.

Anyone else care to weigh in?

michaelplatings added a comment.EditedNov 23 2018, 1:39 AM

@marsupial that's odd. I'm using ninja also and libDynamicLibraryLib.a ends up in lib for me. Reproducer:

svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm
mkdir build && cd build
cmake -G Ninja ../llvm
ninja DynamicLibraryLib
ls lib/libDynamicLibraryLib.a

In case it makes any difference, I used CMake version 3.5.1 and ninja 1.6.0.

(moved closing parenthesis to new line)

@marsupial if you can show me an instance where this patch significantly changes the build's behaviour then I can change the patch to mitigate that.

marsupial accepted this revision.Nov 27 2018, 8:55 PM

Crap your right...I might revisit this (as I think that's a bad move on my part).
I'd still like to give a moment for anyone else to chime in; otherwise, it shouldn't be a reason to hold this up.

This revision is now accepted and ready to land.Nov 27 2018, 8:55 PM
This revision was automatically updated to reflect the committed changes.