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 Init Omnibus #7344

Merged
merged 37 commits into from
May 26, 2021
Merged

Cabal Init Omnibus #7344

merged 37 commits into from
May 26, 2021

Conversation

emilypi
Copy link
Member

@emilypi emilypi commented Mar 30, 2021


Please include the following checklist in your PR:

New additions

This PR does the following:

  • Completely rewrites the cabal init command to have a saner structure that can be tested easily.
  • Optimizing for individual component target generation allows us to golden test individual stanzas under various scenarios instead of just whole cabal files
  • Coverage reports may now be generated for every component of the command. Coverage works for the whole project now, full stop.
  • Control flow for interactive vs. non-interactive vs. simple target generation is completely decoupled.
  • Cleans up the file creation spaghetti, and prunes unused or redundant behavior wherever it arises.
  • Rewrites the prompt code to be less unnecessarily typed (strings are fine!)

This comes with bugfixes:

@emilypi emilypi changed the title Cabal Init Rewrite Cabal Init Omnibus Mar 30, 2021
@phadej
Copy link
Collaborator

phadej commented Mar 30, 2021

Why bundle coverage changes and cabal init changes? Are they related? I don't see how.

@phadej
Copy link
Collaborator

phadej commented Mar 30, 2021

I don't like how this contains at least three separate changes in one PR. I hope this is just a draft.

@emilypi
Copy link
Member Author

emilypi commented Mar 30, 2021

@phadej it's clearly marked a draft. I'll split it out into several PRs if that makes you less uneasy.

Why bundle coverage changes and cabal init changes? Are they related? I don't see how.

Because i can't test the init script reasonably well if i can't generate coverage. It turns out that in order to make any effective change towards fixing things for cabal init, I needed to test it. And then I found out that testing is woefully underserved due to the way the project was structured. Then one thing led to another, and this is the minimal changeset needed to get things working properly so that i can generate coverage for the unit tests we're writing and actually fix cabal init problems with any level of certainty.

@emilypi
Copy link
Member Author

emilypi commented Apr 14, 2021

@fgaz just so @ptkato and I can parallelize a bit, how do you want to split this PR up into reviewable chunks?

@fgaz
Copy link
Member

fgaz commented Apr 14, 2021

At the commit level, ideally the smallest chunks that make sense (and build) on their own. At the pr level, strongly related changes can be on the same pr. Or not. So something like this, following OP:

  • PR: infrastructure stuff (should probably be a single pr since most of the stuff depends on the previous)
    • commit: Expose modules as part of a cabal-install library
    • commit: Gets rid of the dogfooding framework...
    • commit: generate coverage reports
    • commit: Move cabal-install-solver out to its own directory.
    • commit: Move all long-running tests to their own test target
  • PR: fix for HPC generation (or was this the pr that just got merged?)
  • PR: init stuff
    • no idea if this can be split. probably not?

@emilypi
Copy link
Member Author

emilypi commented Apr 14, 2021

@fgaz i agree with that timeline. The HPC + coverage stuff is covered by Ocharles' PR that just went in, and the init stuff can't be split (or at least, shouldn't)

* Restructures the `cabal init` command to fix historical
  issues. All flags are preserved.
  * Codebases for interactive and non-interactive flags
    are disentangled.
  * Data structures now exploit relevant stanza structure
    and formatters only care about stanza data
  * Heuristics and prompts have a pure and impure implementation.

* Sets default behavior to be `--interactive` as opposed to
  `--non-interactive`.

* Rewrites tests to achieve 98% coverage
  * Golden files now test every stanza individually
  * Every flag is covered by a unit test
  * Interactive, simple, and non-interactive workflows are
    covered.
@emilypi emilypi force-pushed the cabal-init-rewrite branch from 17e31e9 to 1068755 Compare May 4, 2021 18:09
emilypi added a commit to ptkato/cabal that referenced this pull request May 4, 2021
@emilypi emilypi marked this pull request as ready for review May 4, 2021 18:14
@emilypi emilypi requested review from Mikolaj and fgaz May 4, 2021 18:14
@emilypi emilypi force-pushed the cabal-init-rewrite branch from 972ceb3 to 5892f42 Compare May 4, 2021 18:41
-> InstalledPackageIndex
-> SourcePackageDb
-> InitFlags
-> m ProjectSettings
createProject v pkgIx srcDb initFlags = do
createProject v _comp pkgIx srcDb initFlags = do
Copy link
Member Author

@emilypi emilypi May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptkato This is unused in a bunch of cases (interactive + simple, including thier tests). I'd just configure NonInteractive.createProject to take the compiler arg first and then return NonInteractive.createProject comp as the function returned in the where clause. Otherwise it just introduces cruft into more places than we want.

@ptkato ptkato force-pushed the cabal-init-rewrite branch from 582d4c3 to 0d20866 Compare May 14, 2021 04:32
@emilypi emilypi requested a review from Mikolaj May 16, 2021 20:46
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome mega-PR. I can't do it justice, not taking part in the epic struggle from the beginning, but I'm duly impressed by its breadth and the law and order it imposes. I would feel uncomfortable with such a large PR, but I understand it provides an excellent test coverage for what it changes. Great job.

@emilypi emilypi merged commit a39d590 into haskell:master May 26, 2021
@emilypi
Copy link
Member Author

emilypi commented May 26, 2021

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment