Testsuite driver: fix encoding issue when calling ghc-pkg
ClosedPublic

Authored by monoidal on Aug 5 2018, 12:38 PM.

Details

Summary

In Python 3, subprocess.communicate() returns a pair of bytes, which
need to be decoded. In runtests.py, we were just calling str() instead,
which converts b'x' to "b'x'". As a result, the loop that was checking
pkginfo for lines starting with 'library-dirs' couldn't work.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
monoidal created this revision.Aug 5 2018, 12:38 PM

To see the difference in behavior, you can add the following debugging output and run tests before and after the patch:

diff --git a/testsuite/driver/runtests.py b/testsuite/driver/runtests.py
index 03f4630510..aa8a8d4975 100644
--- a/testsuite/driver/runtests.py
+++ b/testsuite/driver/runtests.py
@@ -191,6 +191,7 @@ def format_path(path):
 if windows or darwin:
     pkginfo = getStdout([config.ghc_pkg, 'dump'])
     topdir = config.libdir
+    raise Exception([line for line in pkginfo.split('\n') if line.startswith('library-dirs:')])
     if windows:
         mingw = os.path.abspath(os.path.join(topdir, '../mingw/bin'))
         mingw = format_path(mingw)

I'm not sure if this change has any real impact on tests, but it's a clear bug.

thomie added a reviewer: Phyx.Aug 5 2018, 1:49 PM
thomie added a subscriber: Phyx.

Nice find.

the loop that was checking pkginfo for lines starting with 'library-dirs' couldn't work.

Curious that this wasn't caught before.

Maybe the code isn't need at all?

@Phyx?

Phyx added a comment.Aug 5 2018, 3:31 PM

Curious that this wasn't caught before.

dynamic way hasn't been supported on Windows for years.

Maybe the code isn't need at all?

I haven't really started looking at the testsuite yet as dynamic way is on hold for other tasks.

thomie accepted this revision.Aug 5 2018, 3:45 PM

Ah, ok, that makes sense.

Well the patch is an improvement. Maybe it fixes something on osx.

This revision is now accepted and ready to land.Aug 5 2018, 3:45 PM
This revision was automatically updated to reflect the committed changes.