Skip to content

(Try to) add GHC 8.0.1 to the test-matrix #3197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 6, 2016

Conversation

hvr
Copy link
Member

@hvr hvr commented Feb 28, 2016

No description provided.

@hvr
Copy link
Member Author

hvr commented Feb 28, 2016

Package Tests

  BuildDeps/TargetSpecificDeps1:       FAIL (0.59s)
    error should be "Could not find module `Text\.PrettyPrint"

  BuildDeps/TargetSpecificDeps3:       FAIL (0.93s)
    error should be "Could not find module `Text\.PrettyPrint"

I suspect this is because GHC8 now prints a static CallStack... need to investigate

@23Skidoo
Copy link
Member

That test should show actual output in addition to expected.

23Skidoo added a commit that referenced this pull request Feb 28, 2016
@23Skidoo
Copy link
Member

That test should show actual output in addition to expected.

Fixed.

@ezyang
Copy link
Contributor

ezyang commented Feb 28, 2016

Anyway, the error is due to this change in GHC: https://ghc.haskell.org/trac/ghc/ticket/11256#ticket so we should just adjust the message.

@grayjay
Copy link
Collaborator

grayjay commented Feb 28, 2016

The integration tests failed with the same message as in #3059. I think it might be caused by the change to canonicalizePath in directory-1.2.3.0: http://hackage.haskell.org/package/directory-1.2.5.1/changelog

@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

@grayjay as I haven't been following this too closely, what's the recommended course of action? Does Cabal need to be adapted or does directory need to?

@grayjay
Copy link
Collaborator

grayjay commented Feb 29, 2016

@grayjay as I haven't been following this too closely, what's the recommended course of action? Does Cabal need to be adapted or does directory need to?

The problem is that canonicalizePath now succeeds when given a non-existent path, but the integration tests test the error message given by cabal sandbox delete-source p when p does not exist. We could change cabal or its tests. IMO, the ability to remove a non-existent source from a sandbox is an improvement, so we should change the tests.

/cc @23Skidoo

23Skidoo added a commit that referenced this pull request Mar 1, 2016
@grayjay
Copy link
Collaborator

grayjay commented Mar 3, 2016

#3206 fixes the integration test issue.

@hvr hvr force-pushed the pr/ghc8-travis branch from 58ba375 to 3a43778 Compare March 3, 2016 21:04
@hvr
Copy link
Member Author

hvr commented Mar 3, 2016

@23Skidoo seems like cabal check causes the build to fail as well:

$ cabal check
These warnings may cause trouble when distributing the package:
* From version 1.23 cabal supports specifiying explicit dependencies for
Custom setup scripts. Consider using cabal-version >= 1.23 and adding a
'custom-setup' section with a 'setup-depends' field that specifies the
dependencies of the Setup.hs script itself. The 'setup-depends' field uses the
same syntax as 'build-depends', so a simple example would be 'setup-depends:
base, Cabal'.

The command "cabal check" exited with 1.

@23Skidoo
Copy link
Member

23Skidoo commented Mar 3, 2016

@hvr Changed the type of that warning to PackageDistSuspiciousWarn.

@hvr hvr force-pushed the pr/ghc8-travis branch from 3a43778 to 9032f1a Compare March 3, 2016 22:45
@hvr
Copy link
Member Author

hvr commented Mar 3, 2016

@23Skidoo I just pushed some minor fixes, and rebased this PR... maybe it works out this time :-)

@hvr hvr force-pushed the pr/ghc8-travis branch from 9032f1a to 6a89dc7 Compare March 4, 2016 21:11
@hvr
Copy link
Member Author

hvr commented Mar 5, 2016

@23Skidoo now there's a few package tests failing only for GHC8 such as

Package Tests
  BuildDeps/TargetSpecificDeps1:       FAIL (0.57s)
    error should be "Could not find module `Text\.PrettyPrint",
    actual output:
    Component build order: library, executable 'lemon'
    creating dist-test/dist/build
    creating dist-test/dist/build/autogen
    Building TargetSpecificDeps1-0.1...
    /opt/ghc/8.0.1/bin/ghc-pkg init dist-test/./dist/package.conf.inplace
    Preprocessing library TargetSpecificDeps1-0.1...
    Building library...
    creating dist-test/dist/build
    /opt/ghc/8.0.1/bin/ghc --make -fbuilding-cabal-package -static -dynamic-too -dynosuf dyn_o -dynhisuf dyn_hi -outputdir dist-test/./dist/build -odir dist-test/./dist/build -hidir dist-test/./dist/build -stubdir dist-test/./dist/build -i -idist-test/./dist/build -i. -idist-test/./dist/build/autogen -Idist-test/./dist/build/autogen -Idist-test/./dist/build -optP-include -optPdist-test/./dist/build/autogen/cabal_macros.h -this-unit-id TargetSpecificDeps1-0.1-K82WIfV0DY9ER5QSWMrD1c -hide-all-packages -package-db /home/travis/build/haskell/cabal/Cabal/dist/package.conf.inplace -package-db dist-test/./dist/package.conf.inplace -package-id base-4.9.0.0 -package-id bytestring-0.10.7.0 -XHaskell98 MyLibrary
    [1 of 1] Compiling MyLibrary        ( MyLibrary.hs, dist-test/dist/build/MyLibrary.o )

    MyLibrary.hs:4:1: error:
        Failed to load interface for ‘Text.PrettyPrint’
        It is a member of the hidden package ‘pretty-1.1.3.2’.
        Perhaps you need to add ‘pretty’ to the build-depends in your .cabal file.
        Use -v to see a list of the files searched for.

but I'm not sure why the test-case fails... as it seems to behave as supposed to...

PS: is it just the regexp needing updating?

  -- Test "new build-dep behavior", where each target gets
  -- separate dependencies.  This tests that an executable
  -- dep does not leak into the library.
  tc "BuildDeps/TargetSpecificDeps1" $ do
      cabal "configure" []
      r <- shouldFail $ cabal' "build" []
      assertRegex "error should be in MyLibrary.hs" "^MyLibrary.hs:" r
      assertRegex
        "error should be \"Could not find module `Text\\.PrettyPrint\""
        "Could not find module.*Text\\.PrettyPrint" r

@ezyang
Copy link
Contributor

ezyang commented Mar 5, 2016

Yeah the regex needs updating.

@hvr
Copy link
Member Author

hvr commented Mar 5, 2016

do we want a regexp that matches all GHC versions?

@hvr hvr added the test-suite label Mar 5, 2016
@ezyang
Copy link
Contributor

ezyang commented Mar 5, 2016

I can't imagine why not.

Alternately, I could ninja a fix to GHC 8 which reverts the error message to its pre GHC 8.0 behavior.

@hvr
Copy link
Member Author

hvr commented Mar 5, 2016

@ezyang does the error message in GHC8 need fixing? It seemed fine to me as it is...

@bgamari
Copy link
Contributor

bgamari commented Mar 5, 2016

I'll admit that the mention of interface files here was a bit surprising to me the first time I saw this message.

garetxe pushed a commit to garetxe/cabal that referenced this pull request Mar 5, 2016
@ezyang
Copy link
Contributor

ezyang commented Mar 5, 2016

I think for anyone who has done any modicum of Haskell development, the error is fine. For absolute beginners the mention of an interface file may be slightly confusing, but not more so than skolem variables, type classes, etc.

@hvr
Copy link
Member Author

hvr commented Mar 5, 2016

@ezyang ok, but is the error-message actually accurate? i.e. does GHC really miss the .hi file rather than the .hs file?

@ezyang
Copy link
Contributor

ezyang commented Mar 5, 2016

Yes, it is in fact the hi file missing that is prompting the error. The error changed because we made it no longer an error for ghc --make to operate without finding all of the source files. I guess maybe the right way to change this is to have ghc --make report a warning that compilation is definitely going to fail when it discovers, maybe.

hvr added 2 commits March 6, 2016 10:05
Currently GHC 8.0 has a slightly different error message if it can't
find a `.hi` file. It's not clear yet if and how we're going to change the
message before GHC 8.0 final.
@hvr hvr force-pushed the pr/ghc8-travis branch from 6a89dc7 to b45ecc8 Compare March 6, 2016 09:06
@hvr
Copy link
Member Author

hvr commented Mar 6, 2016

@23Skidoo fyi, Travis is finally green (including GHC HEAD)!

@23Skidoo
Copy link
Member

23Skidoo commented Mar 6, 2016

@hvr Great!

23Skidoo added a commit that referenced this pull request Mar 6, 2016
(Try to) add GHC 8.0.1 to the test-matrix
@23Skidoo 23Skidoo merged commit 52661a7 into haskell:master Mar 6, 2016
@23Skidoo
Copy link
Member

23Skidoo commented Mar 6, 2016

Merged, thanks!

@hvr hvr deleted the pr/ghc8-travis branch March 6, 2016 22:04
@23Skidoo
Copy link
Member

23Skidoo commented Mar 7, 2016

Also cherry-picked into 1.24.

@23Skidoo
Copy link
Member

23Skidoo commented Mar 7, 2016

Also cherry-picked into 1.24.

Oh well, now the 1.24 build fails with:

Distribution/Client/FetchUtils.hs:159:36: error:
    • Couldn't match type ‘Distribution.Package.PackageIdentifier’
                     with ‘[email protected]:Distribution.Package.PackageIdentifier’
      NB: ‘[email protected]:Distribution.Package.PackageIdentifier’
            is defined in ‘Distribution.Package’
                in package ‘[email protected]’
          ‘Distribution.Package.PackageIdentifier’
            is defined in ‘Distribution.Package’
                in package ‘[email protected]’
      Expected type: [email protected]:Distribution.Package.PackageIdentifier
        Actual type: PackageId

    • In the second argument of ‘Sec.downloadPackage'’, namely ‘pkgid’
      In a stmt of a 'do' block: Sec.downloadPackage' rep pkgid path
      In the second argument of ‘($)’, namely
        ‘do { info verbosity ("writing " ++ path);
              Sec.downloadPackage' rep pkgid path }’

So for some reason hackage-security was compiled against a different version of Cabal-1.24.0.0 than cabal-install (there's the one that's built by us and the one that comes with GHC 8). The question is of course why the solver does that. It seems to be able to detect that something went wrong:

Warning: This package indirectly depends on multiple versions of the same
package. This is highly likely to cause a compile failure.
package hackage-security-0.5.0.2 requires Cabal-1.24.0.0
package cabal-install-1.24.0.0 requires Cabal-1.24.0.0

@kosmikus, do you think we're dealing with a bug in the solver here?

@ezyang
Copy link
Contributor

ezyang commented Mar 7, 2016

I have noticed occasionaly that if Cabal/GHC doesn't remember to recompile enough of hackage-security, this situation can arise. I haven't diagnosed if it's a bug in GHC's recompilation avoidance or Cabal. If you clean out your hackage-security build that should fix the problem?

@23Skidoo
Copy link
Member

23Skidoo commented Mar 7, 2016

If you clean out your hackage-security build that should fix the problem?

This happens on Travis, so hackage-security is built from scratch (we run cabal install --only-dependencies).

@dcoutts
Copy link
Contributor

dcoutts commented Mar 9, 2016

@23Skidoo iirc, the travis build script does not use cabal install to build things, but instead install deps then does configure & build. With the way configure currently works it cannot change its environment so it can end up with that warning about indirect deps on multiple versions.

Incidentally this class of failure is solved by the nix-local-build branch, since it solves for everything even if you're just running configure, so configure+build can solve and install deps.

@grayjay
Copy link
Collaborator

grayjay commented Mar 9, 2016

Should cabal install never include two versions of a package in one install plan (ignoring independent goals)? When we used the Haskell Platform for the Appveyor build, cabal install --only-dependencies --enable-tests --force-reinstalls printed a warning about multiple versions: https://ci.appveyor.com/project/23Skidoo/cabal/build/1.0.80

@23Skidoo
Copy link
Member

23Skidoo commented Mar 9, 2016

@dcoutts What I don't understand is why configure picked a different Cabal-1.24 snapshot for compiling cabal-install than install --only-dependencies did for hackage-security. Ideally the fresh one from the user package DB one would be picked both times; at the very least I'd expect the choice to be consistent in both cases.

@dcoutts
Copy link
Contributor

dcoutts commented Mar 9, 2016

@23Skidoo if it's Setup configure you're using then it's not guaranteed to pick the same things as cabal install --deps did. Using cabal install --deps followed by cabal configure should pick the same (consistent) things. But Setup configure just picks the latest of everything.

@23Skidoo
Copy link
Member

@dcoutts Thanks, that's indeed the problem. Checking now whether we can use cabal configure instead (may break because LBI is unreadable)...

@23Skidoo
Copy link
Member

And it looks like we can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants