haskell / attoparsec

A fast Haskell library for parsing ByteStrings
http://hackage.haskell.org/package/attoparsec
Other
514 stars 93 forks source link

Remove internal library #210

Closed tfausak closed 1 year ago

tfausak commented 2 years ago

This pull request removes the internal library that was added in #199. The modules that were exposed by the internal library were moved into the Data.Attoparsec.Internal.* namespace (if they weren't there already). Aside from that, there are no other changes.

I was motivated to do this because Cabal does not support code coverage for packages that use internal libraries: https://github.com/haskell/cabal/issues/6440. By switching from an internal library to the conventional internal module, I was able to test my project with code coverage.

tfausak commented 2 years ago

Oh, I forgot to mention: This is the error that I see:

$ cabal test --enable-coverage
Error:
    Internal libraries only supported with per-component builds.
    Per-component builds were disabled because program coverage is enabled
    In the package 'attoparsec-0.14.4'
Bodigrim commented 2 years ago

This is a bug in tooling and there is a workaround available, so from my perspective changing public API is not quite justified. If there were no workaround, a better solution would be to expose internal library as a public one, uploading it on Hackage separately.

tfausak commented 2 years ago

Yes, there is a workaround. For posterity, here it is:

-- cabal.project.local
package *
  coverage: True
  library-coverage: True

package attoparsec
  coverage: False
  library-coverage: False

I figured I would open this PR since exposing some internal modules didn't seem like a big deal to me and it would make the workaround unnecessary.

tfausak commented 2 years ago

For what it's worth, the ergonomics of the workaround aren't very good. I had hoped that you could put the package attoparsec section in your cabal.project, then either enable or disable coverage as desired. Unfortunately that doesn't work; you'll still get the error from Cabal even if you have that section in your cabal.project. So if you want to build without coverage you have to remove the workaround from your cabal.project (or cabal.project.local). And then if you want to build with coverage you have to add it back in.

On the other hand if you use this fork as a source-repository-package, you can cabal test --disable-coverage or cabal test --enable-coverage without changing any other configuration.

-- cabal.project
source-repository-package
  type: git
  location: https://github.com/tfausak/attoparsec
  tag: 727c81efdc779a0bd400d77a4452e5b5b0f3d82d
tfausak commented 2 years ago

It looks like another user ran into this problem: https://github.com/haskell/cabal/issues/6440#issuecomment-1120314764

I understand that you do not want to merge this change. Is there anything I can do, or indeed anything at all, that could change your mind?

Bodigrim commented 2 years ago

@tfausak I'm not a maintainer here, but making attoparsec-internal a public package, as suggested above in https://github.com/haskell/attoparsec/pull/210#issuecomment-1098343599, is the best and the simplest option in my opinion. It's just a matter of adding an additional cabal file.

tfausak commented 2 years ago

Oops, my bad. Not sure why I thought you were a maintainer.

I think @bgamari is the only maintainer?

tfausak commented 1 year ago

I'm closing this since haskell/cabal#6440 is the real fix and I suspect that packages with multiple libraries are only going to become more popular.