Fix #4270: make detailed-0.9 test suites work with dynamic exes

[bennofs]

The detailed-0.9 test suite type compiles the test module into a shared library,
so for running the tests, we need to include the directory where the shared
library is stored in LD_LIBRARY_PATH.

[mention-bot]

@bennofs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ericson2314, @ttuegel and @ezyang to be potential reviewers.

[ttuegel]

Do we need to do the same thing for exitcode-stdio-1.0 tests?

[23Skidoo]

@ttuegel Mayhaps. Does the fix look good to you?

@bennofs Can you please add a regression test?

[bennofs]

@ttuegel I believe not since a normal executable component will not build any shared library that needs to be linked against

[ttuegel]

I believe not since a normal executable component will not build any shared library that needs to be linked against

@bennofs What if it links against the package's library component?

[ezyang]

From AppVeyor failure, test needs to be disabled on platforms that don't support dynamic linking e.g. Windows. Use skipUnless =<< hasSharedLibraries

[ezyang]

I don't think you wanted to keep this

[ezyang]

Are you sure you want canonicalizePath and not makeAbsolute?

[bennofs]

@ttuegel That is already handled correctly, since we do add the library dirs for all dependencies. The LibV09 case just wasn't handled correctly because for this particular case, we need the libdir of the current component as well (since that contains the shared lib that is loaded by the test stub), not just of the dependencies.

I (hopefully) addressed the remaining review comments (thanks for that!). Tell me if you'd like me to squash the commits.

[bennofs]

Should hasSharedLibraries be false when the Cabal library itself is not build with shared libraries? This is the reason why it's failing on travis right now: for some build configs on travis, we only build the static Cabal library so the test cannot run.

[ezyang]

@bennofs You're right! So I guess we also need a "has Cabal shared library" parameter. Unfortunately, I don't actually know how to test for this... maybe do a ./configure style probe? (Don't do it on every test, only when requested; see withDelay for an example of how we've been coding this sort of thing.)

[bennofs]

This PR actually seems to be a duplicate of #3527

[bennofs]

@ezyang I was looking at the test code, and found a function called hasCabalGorGhc in Prelude.hs. What's that function for?

[ezyang]

hasCabalForGhc is a different function, related to what we do when we test the Cabal library, passing --with-ghc for a different version of GHC. In this situation, we can't use certain features like detailed test suites, because the version of GHC we are compiling the test with doesn't have the new version of Cabal.

[bennofs]

I squashed some of the commits, travis build should be fine now.

new changes

[ezyang]

This is an interesting implementation of hasCabalShared. At the very least, I would add a note that it is a relatively expensive check, because it involves banging on GHC, so you really only want to call this once, if at all possible.

Assuming that this is the best way to go about doing this, I have some comments:

• First, are you sure that runProgramM ghcProgram isn't enough to invoke GHC? I believe that GHC should already be configured in testProgramDb; the configuration was done in Test/Cabal/Monad.hs so I don't think we should have to do it again. If this doesn't work, there is probably a different bug somewhere. Is the problem that runM calls requireSuccess whereas you need to get a boolean out instead? Maybe we can refactor those functions so you get a version that raises exceptions, and one that doesn't. This is related to cabal-testsuite: check return code of runghc Setup.hs #4292 where we wanted to handle fails more systematically.

• Second, the invocation of GHC doesn't attempt to control what packages are in scope, which means that you could end up in a situation where there are two Cabal packages in scope. I guess GHC is preferring the latest version so it's working correctly in this case, but it would be more robust to explicitly specify which Cabal library we want to use, and the test would stop working the moment someone uses a GHC that comes with a newer version of Cabal.

Is this check the way we want to go about it? I'll admit, I can't think of very many other good choices. One possibility is to look at the LocalBuildInfo when we mkScriptEnv and check if dynamic compilation was enabled (adding a new field to ScriptEnv saying if we were building with dynamic or not.) This seems a lot simpler and efficient!

[ezyang]

Sorry about the yak shaving on hasCabalShared; I know I'm the one who suggested the configure style check, but I only just realized that maybe the LBI has the info we want. If that method works, I think it's my preferred choice. But even if it doesn't, I hope we can simplify the code in hasCabalShared a bit.

[bennofs]

Using the LocalBuildInfo sounds like a good idea indeed, I will have to try that.

[bennofs]

Windows build failure is due to #4005

[bennofs]

@ezyang using the LocalBuildInfo was a really good idea, there's now much less code.

[ezyang]

@bennofs Good! I restarted the Windows build, if it passes let's ship it!

[ezyang]

Failing -Werror; import changes need to be undone.

[bennofs]

@ezyang fixed that and squashed the changes into the add-regression-test commit.

[ezyang]

Good work!