Add test build directory to LD_LIBRARY_PATH when -executable-dynamic #3527
Conversation
Refactoring only. This is necessary for the next commit. To prevent import cycles, I moved `checkForeignDeps` from `Cabal.Distribution.Simple.Configure` to `Cabal.Distribution.Simple`.
Since commit 18fcd9c, Cabal builds the stub executable and the test library for a LibV09 (detailed-0.9) test in different directories. This is good, but when running the test with --enable-executable-dynamic, we now have to add the test build directory to (DY)LD_LIBRARY_PATH, so that the linker can find the test library when running the stub executable (see haskell#2289 for details). This fixes haskell#2039 (read: no parse). The GHC panic mentioned in that ticket must have been resolved earlier already, perhaps also by commit 18fcd9c.
| paths <- LBI.depLibraryPaths | ||
| True False lbi clbi | ||
| True False lbi exeClbi |
There was a problem hiding this comment.
This needs a comment explaining why it's exeClbi and not clbi.
There was a problem hiding this comment.
I added the following comment already:
-- Get the dependencies of the stub
-- executable. They should include the test
-- library itself (#2039).
If you still think it isn't clear, could you suggest what the comment should be?
There was a problem hiding this comment.
I investigated and here is what I discovered:
- Here is something confusing: the function is called
depLibraryPaths(library?!), yet we are passing it the CLBI for the executable! I looked at the definition and the point is that depLibraryPaths doesn't have anything to do about libraries; it has to do with the DEPENDENT libraries, and whose dependencies do we care about? The executables. So I guess I'd personally be a fan of a-- NB: the library path is for the dependencies of the *executable*, not library(or if you want to shave yaks, renamingdepLibraryPaths) - I don't understand why this code has to go through all these gyrations to even get its hands on the executable CLBI. This seems wrong-headed. The real problem is that the CLBIs for this type of test is a massive hack, as explained in Build:
clbi -- This ComponentLocalBuildInfo corresponds to a detailed
-- test suite and not a real component. It should not
-- be used, except to construct the CLBIs for the
-- library and stub executable that will actually be
-- built.
A variant of this comment seems apt here. If this were GHC I'd tell you to refer to the Note.
3. And here is another yak: there is probably no good reason why runTest shouldn't be taking in the ComponentLocalBuildInfo. build certainly does! (But this is a yak and you do not have to shave it.)
Does that help?
|
LGTM, I think this can go in once @ezyang's comments are addressed. |
…g failures This change is an attempt to address failures like the one at https://travis-ci.org/jtdaugherty/vty/jobs/660259959 that give rise to the "Prelude.read: no parse" errors. Although that may not seem like the error one would expect, see these resources for details on why I think this is the "fix" for this problem: * https://stackoverflow.com/questions/39310043/getting-prelude-read-no-parse-error-when-using-detailed-0-9-tests-with-cabal * haskell/cabal#3527
|
@thomie: hi! it's been a while and the code must have bitrotted, but AFAICT it's still an important issue to solve, so would you like to work on it and merge it? |
|
@Mikolaj: sorry, I will not work on this anymore. |
|
@thomie: that's fine, thank you for your contributions and all the best! |
Since commit 18fcd9c, Cabal builds the stub executable and the test library for a LibV09 (detailed-0.9) test in different directories.
This is good, but when running the test with --enable-executable-dynamic, we now have to add the test build directory to (DY)LD_LIBRARY_PATH, so that the linker can find the test library when running the stub executable (see #2289 for details).
This fixes #2039 (read: no parse). The GHC panic mentioned in that ticket must have been resolved earlier already, perhaps also by commit 18fcd9c.
CC @ezyang, @christiaanb