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

Remove internal library #210

Closed
wants to merge 2 commits into from

Conversation

tfausak
Copy link

@tfausak tfausak commented Apr 13, 2022

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: haskell/cabal#6440. By switching from an internal library to the conventional internal module, I was able to test my project with code coverage.

@tfausak
Copy link
Author

tfausak commented Apr 13, 2022

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
Copy link
Contributor

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
Copy link
Author

tfausak commented Apr 13, 2022

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
Copy link
Author

tfausak commented Apr 13, 2022

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
Copy link
Author

tfausak commented May 8, 2022

It looks like another user ran into this problem: haskell/cabal#6440 (comment)

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
Copy link
Contributor

Bodigrim commented May 8, 2022

@tfausak I'm not a maintainer here, but making attoparsec-internal a public package, as suggested above in #210 (comment), is the best and the simplest option in my opinion. It's just a matter of adding an additional cabal file.

@tfausak
Copy link
Author

tfausak commented May 8, 2022

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

I think @bgamari is the only maintainer?

@tfausak
Copy link
Author

tfausak commented Jul 27, 2023

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.

@tfausak tfausak closed this Jul 27, 2023
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