Skip to content
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

Add dualizer & yaya. #4314

Merged
merged 1 commit into from
Jan 20, 2019
Merged

Add dualizer & yaya. #4314

merged 1 commit into from
Jan 20, 2019

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Jan 20, 2019

Checklist:

  • Meaningful commit message, eg add my-cool-package (please not mention build-constraints.yml)

  • At least 30 minutes have passed since uploading to Hackage

  • On your own machine, in a new directory, you have successfully run the following set of commands (replace $package with the name of the package that is submitted, and $version with the version of the package you want to get into Stackage):

    stack unpack $package-$version
    cd $package-$version
    stack init --resolver nightly
    stack build --resolver nightly --haddock --test --bench --no-run-benchmarks
    

@mihaimaruseac mihaimaruseac merged commit 2a054f5 into commercialhaskell:master Jan 20, 2019
@mihaimaruseac
Copy link
Contributor

Thank you

@mihaimaruseac
Copy link
Contributor

There are test failures, so I'm going to rever this for now.

Building test suite 'yaya-test' for yaya-0.2.1.0..
[1 of 5] Compiling Test.Fold        ( test/Test/Fold.hs, dist/build/yaya-test/yaya-test-tmp/Test/Fold.o )
[2 of 5] Compiling Test.Fold.Common ( test/Test/Fold/Common.hs, dist/build/yaya-test/yaya-test-tmp/Test/Fold/Common.o )

test/Test/Fold/Common.hs:15:44: error:
     No instance for (Recursive (yaya-0.2.1.0:Yaya.Fold.Mu Expr) f0)
        arising from a use of cata
     In the second argument of (.), namely
        cata (zipAlgebras height size)
      In the second argument of (.), namely
        fmap toInteger . cata (zipAlgebras height size)
      In the second argument of (.), namely
        uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
   |
15 |   (assert . uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
   |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

test/Test/Fold/Common.hs:15:50: error:
     Ambiguous type variable f0 arising from a use of zipAlgebras
      prevents the constraint (Functor f0) from being solved.
      Probable fix: use a type annotation to specify what f0 should be.
      These potential instances exist:
        instance Functor (Either a) -- Defined in ‘Data.Either’
        instance Functor IO -- Defined in ‘GHC.Base’
        instance Functor m => Functor (GenT m)
          -- Defined in ‘Hedgehog.Internal.Gen’
        ...plus 7 others
        ...plus 81 instances involving out-of-scope types
        (use -fprint-potential-instances to see them all)
     In the first argument of cata, namely
        (zipAlgebras height size)
      In the second argument of (.), namely
        cata (zipAlgebras height size)

Let us know if you want to fix the test first or just disable it (it won't be compiled and test dependencies won't be checked)

@sellout
Copy link
Contributor Author

sellout commented Jan 20, 2019

Argh, sorry about that. I’ll sort it out!

@sellout
Copy link
Contributor Author

sellout commented Jan 21, 2019

I can’t replicate this locally, but I’ve seen a similar error report like this before, and I think it’s related to the dependency graph. The familiar part is Recursive (yaya-0.2.1.0:Yaya.Fold.Mu Expr) f0, where some of the names are fully qualified, when there’s no ambiguity.

Here’s the relevant part of the graph.
dependencies

“yaya” and “yaya-hedgehog” are two separate cabal projects. The test suite for yaya depends on the yaya-hedgehog library, which depends on the yaya library, creating a cycle between the two cabal projects (although there is no cycle between the targets).

My assumption is that the project cycle is causing this confusing error somehow. (Confusing because following the code makes it obvious that there is a Recursive (Mu Expr) instance, even though the error is about there not being one.)

So, is there an easy fix for this? If not, maybe disabling the tests is the right call …

@mihaimaruseac
Copy link
Contributor

Can we add yaya-hedgehog too to Stackage?

@sellout
Copy link
Contributor Author

sellout commented Jan 21, 2019

Yeah, yaya-hedgehog is in this PR (as is yaya-unsafe).

Judging from the error

No instance for (Recursive (yaya-0.2.1.0:Yaya.Fold.Mu Expr) f0)

it seems as though the yaya that yaya-hedgehog depends on is (seen as) distinct from the yaya we’re testing. References via yaya-hedgehog (e.g., Mu) have the yaya-0.2.1.0: prefix, while ones from the test suite itself (e.g., Recursive) don’t. And it’s claiming that there is no instance of the test suite’s Recursive type class for yaya-hedgehog’s Mu Expr.

I have no idea how to fix that confusion, though.

@mihaimaruseac
Copy link
Contributor

mihaimaruseac commented Jan 22, 2019

Oh, sorry, I missed it.

There are two possible fixes here:

  • either you enforce bounds in Cabal to all of these packages so they all use correct versions (probably use ==)
  • or you move the tests in yaya to yaya-hedgehog

The first one should help in resolution by ensuring that you have a consistent set of packages always. But will cause troubles since now whenever you update one package you'll have to update bounds for the other too Stackage curators will open an issue each time this happens.

The second option might work since now you break the dependency cycle.

There's also the option of adding the packages one by one, in separate PRs, in topological order. However, I don't think that is really going to work always, I give more chances to the above two solutions I tested this and it's not working.

@mihaimaruseac
Copy link
Contributor

Another thing: this might also be a Stack bug, if you are able to reproducibly build everything with cabal but not with Stack.

I tried the following set of commands locally

$ stack unpack yaya
$ stack unpack yaya-hedgehog
$ stack init --resolver=nightly
$ stack test

All from the same directory. The last one failed with 3 errors:

    /tmp/yaya/yaya-0.2.1.0/test/Test/Fold/Common.hs:15:44: error:
         No instance for (Recursive (yaya-0.2.1.0:Yaya.Fold.Mu Expr) f0)
            arising from a use of cata
         In the second argument of (.), namely
            cata (zipAlgebras height size)
          In the second argument of (.), namely
            fmap toInteger . cata (zipAlgebras height size)
          In the second argument of (.), namely
            uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
       |
    15 |   (assert . uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
       |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
    /tmp/yaya/yaya-0.2.1.0/test/Test/Fold/Common.hs:15:50: error:
         Ambiguous type variable f0 arising from a use of zipAlgebras
          prevents the constraint (Functor f0) from being solved.
          Probable fix: use a type annotation to specify what f0 should be.
          These potential instances exist:
            instance Functor (Either a) -- Defined in ‘Data.Either’
            instance Functor IO -- Defined in ‘GHC.Base’
            instance Functor m => Functor (GenT m)
              -- Defined in ‘Hedgehog.Internal.Gen’
            ...plus 7 others
            ...plus 81 instances involving out-of-scope types
            (use -fprint-potential-instances to see them all)
         In the first argument of cata, namely
            (zipAlgebras height size)
          In the second argument of (.), namely
            cata (zipAlgebras height size)
          In the second argument of (.), namely
            fmap toInteger . cata (zipAlgebras height size)
       |
    15 |   (assert . uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
       |                                                  ^^^^^^^^^^^^^^^^^^^^^^^
    
    /tmp/yaya/yaya-0.2.1.0/test/Test/Fold/Common.hs:15:62: error:
         Ambiguous type variable f0 arising from a use of height
          prevents the constraint (Foldable f0) from being solved.
          Probable fix: use a type annotation to specify what f0 should be.
          These potential instances exist:
            instance Foldable (Either a) -- Defined in ‘Data.Foldable’
            instance Foldable Expr -- Defined in ‘Yaya.Hedgehog.Expr’
            instance Foldable Maybe -- Defined in ‘Data.Foldable’
            ...plus two others
            ...plus 57 instances involving out-of-scope types
            (use -fprint-potential-instances to see them all)
         In the first argument of zipAlgebras, namely height
          In the first argument of cata, namely (zipAlgebras height size)
          In the second argument of (.), namely
            cata (zipAlgebras height size)
       |
    15 |   (assert . uncurry (<) . fmap toInteger . cata (zipAlgebras height size)
       |                                                              ^^^^^^
    

@sellout
Copy link
Contributor Author

sellout commented Jan 22, 2019

Thanks! I can finally replicate this issue locally.

Because the PR instructions say to cd after stack unpack, I didn’t realize you could unpack multiple packages so easily. And thanks for the suggestions on how to fix it. New PR coming ASAP 🤞🏼

@sellout
Copy link
Contributor Author

sellout commented Jan 22, 2019

Also, I can build it with stack from the repo that contains all of this (sellout/yaya), but not when I use stack unpack to grab the Hackage-published versions.

@mihaimaruseac
Copy link
Contributor

Oh, true, the instructions are for including a single package, but it seems even in that case you don't need to cd. Going to make a PR to change this soon.

I think the difference between repo and Hackage version is that you already have fixed versions in your code, from the repo, in the working dir. That's why we included the instructions to do the testing in a separate directory.

I'm glad we got the end of it, looking forward to the new PR (though probably another curator will act on it)

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.

2 participants