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

Replace Boilerplate Monoid/Semigroup instances with generics #3196

Merged
merged 5 commits into from
Feb 29, 2016

Conversation

hvr
Copy link
Member

@hvr hvr commented Feb 28, 2016

addresses #3169

This is preparatory work for implementing haskell#3169
it's kept in a different commit in order to facilitate
comparing code-generation.
@hvr
Copy link
Member Author

hvr commented Feb 28, 2016

@dcoutts @23Skidoo There's a few instances that we can't rewrite yet without more refactoring. Specifically those which have Bool flags without Data.Monoid.All or Data.Monoid.Any newtype-wrappers to guide the Monoid derivation.

@hvr
Copy link
Member Author

hvr commented Feb 28, 2016

Oh, and the Semigroup instances for TestSuite/Executable/Benchmark are partial via error!

instance Semigroup Executable where
  a <> b = Executable{
    exeName    = combine' exeName,
    modulePath = combine modulePath,
    buildInfo  = combine buildInfo
  }
    where combine field = field a `mappend` field b
          combine' field = case (field a, field b) of
                      ("","") -> ""
                      ("", x) -> x
                      (x, "") -> x
                      (x, y) -> error $ "Ambiguous values for executable field: '"
                                  ++ x ++ "' and '" ++ y ++ "'"

hvr added 2 commits February 28, 2016 09:36
This is preparatory work for implementing haskell#3169
it's kept in a different commit in order to facilitate
comparing code-generation.
This increases compile-time (until GHC becomes more clever) but the
generated code is expected to be at least as good (if not better) than
the manually generated code.

This addresses haskell#3169
@hvr hvr changed the title WIP: Replace Boilerplate Monoid/Semigroup instances with generics Replace Boilerplate Monoid/Semigroup instances with generics Feb 28, 2016
@hvr
Copy link
Member Author

hvr commented Feb 28, 2016

Interesting... the cabal-install changes seem to cause integration-test failures

hvr added 2 commits February 28, 2016 10:54
This increases compile-time (until GHC becomes more clever) but the
generated code is expected to be at least as good (if not better) than
the manually generated code.

While at it, this removes -XCPP usage from all modules touched.

This addresses haskell#3169
@23Skidoo
Copy link
Member

Nice! Boolean flags should use Any, I think.

@hvr
Copy link
Member Author

hvr commented Feb 28, 2016

Nice! Boolean flags should use Any, I think.

there are a few cases which have it inverted:

instance Semigroup Library where
 -- ...
       libExposed     = libExposed a && libExposed b, -- so False propagates
instance Semigroup BuildInfo where
  a <> b = BuildInfo {
    buildable           = buildable a && buildable b,
   -- ...
 }

23Skidoo added a commit that referenced this pull request Feb 29, 2016
Replace Boilerplate Monoid/Semigroup instances with generics
@23Skidoo 23Skidoo merged commit f646376 into haskell:master Feb 29, 2016
@23Skidoo
Copy link
Member

Merged, thanks!

@hvr hvr deleted the pr/issue-3169 branch February 29, 2016 10:00
@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

@23Skidoo what are your plans re 1.24 branch? I mostly want to know to decide what to do about the nix-local branch

@23Skidoo
Copy link
Member

@hvr Well, I didn't want to backport this into 1.24 initially, but it's probably not a very big deal after all. And merges will be easier. I just need to find a way to download all custom setup scripts from Hackage to check that #3195 didn't break anything.

@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

That shouldn't be too hard:

cabal list --simple | awk '{ print $1 }' | uniq

gives you a list of all package names,

and

curl -s --fail http://hackage.haskell.org/package/$PKGNAME/src/Setup.lhs || curl -s --fail http://hackage.haskell.org/package/$PKGNAME/src/Setup.hs

tries to retrieve you the latest version of $PKGNAME's Setup.lhs or Setup.hs

you could also try to be more clever and look at the .cabal files first (e.g. from your 00-index.tar ball) and check for the build-type before downloading the Setup.(l)hs files...

@23Skidoo
Copy link
Member

you could also try to be more clever and look at the .cabal files first (e.g. from your 00-index.tar ball) and check for the build-type before downloading the Setup.(l)hs files...

Yep, that's what I want to do.

@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

In that case:

cd $TMP
tar xf ~/.cabal/packages/hackage.haskell.org/00-index.tar.gz 
find -name '*.cabal' | xargs grep -li '^build-type:.*custom' | cut -f2 -d/ | uniq

@23Skidoo
Copy link
Member

@hvr Thanks! That was easier than I expected. Uploaded the results to https://github.com/23Skidoo/all-custom-setups. And it turns out llvm-general actually uses configPrograms in its setup script.

@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

@23Skidoo I think that specific case in llvm-general can be made more compatible by providing a function configPrograms with the old type-sig (as it's only used as a function, rather than a field-record-label). and name the actual field differently, e.g. configPrograms_

PS: c.f.

-- | More convenient version of 'configPrograms'. Results in an
-- 'error' if internal invariant is violated.
configPrograms' :: ConfigFlags -> ProgramConfiguration
configPrograms' = maybe (error "FIXME: remove configPrograms") id . getLast' . configPrograms

@23Skidoo
Copy link
Member

Yes, it should work given that the script doesn't use explicit import lists.

@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

@23Skidoo btw, @dcoutts tells me he "used to have a script to try compiling all custom Setup.hs scripts with old and new Cabal lib version to look for breakages"

did you try to compile those Setup.hs files or did you just grep for configPrograms?

@23Skidoo
Copy link
Member

@hvr Just grepped.

hvr added a commit to hvr/cabal that referenced this pull request Feb 29, 2016
@hvr
Copy link
Member Author

hvr commented Feb 29, 2016

@23Skidoo fwiw, if you're still worried about merging to 1.24, you could try merging only the cabal-install parts, or we could simply back out the ConfigFlags instance from the part merged to 1.24 (I'll happily prepare a PR if you want the latter variant)

23Skidoo pushed a commit that referenced this pull request Mar 1, 2016
This implements the suggestion from
#3196 (comment)

(cherry picked from commit 0077e2c)
@23Skidoo
Copy link
Member

23Skidoo commented Mar 1, 2016

@hvr Cherry-picked all your patches into 1.24.

garetxe pushed a commit to garetxe/cabal that referenced this pull request Mar 5, 2016
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