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

cabal check improvements for globs #5403

Merged

Conversation

quasicomputational
Copy link
Contributor

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Tests added!

This PR is based on top of #5401 and #5402 (which explains today's PR frenzy: I didn't expect to need to do #5401 when I started!). I'll rebase on master once those two are merged. As noted in #5401, the cabal-version check in Check.hs is kind of weird; I have left it in the expected output of the relevant test on the basis that it's easy to remove later.

@quasicomputational quasicomputational added this to the 2.4 milestone Jun 27, 2018
@quasicomputational quasicomputational force-pushed the glob-missing-directory-error branch from e673f1b to a20ab93 Compare June 27, 2018 13:27
@quasicomputational
Copy link
Contributor Author

This PR actually needs correction on two counts:

  • one, it's a lie to use PackageDistInexcusable here because Hackage won't reject these. The no-empty-glob check could actually be done Hackage-side, though. What's the policy on introducing new ways for Hackage to reject code?

  • two, now that globs are being expanded by cabal check, cabal check will bork if the globs have bad syntax. This isn't new with this PR but I should fix it anyway.

@quasicomputational quasicomputational force-pushed the glob-missing-directory-error branch from a20ab93 to 061073b Compare June 28, 2018 10:33
@quasicomputational
Copy link
Contributor Author

I've fixed the two problems above. I think it'd still make sense to bump up the 'glob matches nothing' and the 'glob failed to parse' warnings to PackageDistInexcusable and having Hackage enforce those, but I don't know the backwards-compatibility story here.

@quasicomputational
Copy link
Contributor Author

Oh, and this will also conflict with #5397, but that's an easy merge; if I'd thought, I would have based this on top of that one too. I'd definitely suggest putting #5397 in first because that's causing some grief for Hadrian.

This also significantly improves the error when trying to refer to
missing directories, hopefully making it clear that it's coming from
Cabal. haskell#5318 and snowleopard/hadrian#634 are two bugs which manifested
as Cabal trying to glob in a non-existent directory and both took some
debugging because of the obscurity of the error.
These globs make `haddock`, `sdist` and `install` die, so they
definitely ought to be warned about!

Because of the dying behaviour, I made the checks dist-inexcusable;
starting from a clean slate I would probably have only made them
suspicious, but this isn't terrible.
This has been a problem since haskell#5372 began expanding globs in `cabal
check`. Now the logic of running a glob is separated from the parsing,
giving the caller the opportunity to handle parsing failures flexibly.
@quasicomputational quasicomputational force-pushed the glob-missing-directory-error branch from 593b670 to 24c4e77 Compare July 8, 2018 12:15
@typedrat
Copy link
Collaborator

typedrat commented Jul 9, 2018

Tested this a bit and read the code. LGTM.

@typedrat typedrat merged commit dcc157c into haskell:master Jul 9, 2018
@quasicomputational
Copy link
Contributor Author

Thanks!

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