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

Tests for solver support language extensions and language flavours #2873

Merged
merged 2 commits into from Nov 26, 2015

Conversation

Projects
None yet
7 participants
@jdnavarro
Copy link
Collaborator

jdnavarro commented Oct 15, 2015

These are the tests and the corresponding DSL modifications required for merging PR #2732. I don't think GitHub lets you reuse PR issue number so that's why I'm submitting a new PR.

Aside of general mistakes, please check I'm not missing any important test case.

I acknowledge the commit history is not very clean, let me know if you want me to clean it up.

/cc @kosmikus @luigibozzo

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Oct 15, 2015

Thanks a lot. I'll try to take a look.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 16, 2015

Nice!

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Oct 16, 2015

Ooooh! Interesting, I'll try to have good gander at this tomorrow. (Not that I decide anything, obviously.)

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Oct 28, 2015

@jdnavarro Sorry for the long delay. Yes, this all looks good as far as I can see. Thanks! I think it would indeed be nice if you could rebase and squash all of the test-related commits (so that in principle there are only two changes in this PR: my solver change and your tests change).

@jdnavarro jdnavarro force-pushed the jdnavarro:lang-extensions branch from 4acce9e to 594335b Oct 29, 2015

@jdnavarro

This comment has been minimized.

Copy link
Collaborator

jdnavarro commented Oct 29, 2015

@kosmikus, I had to redo the merge conflicts we resolved at the hackathon in order to include them in your commit, so please have a look at your your commit too.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 18, 2015

@ "the management" :) :

Any reason we shouldn't just push the "merge" button on this?

We're piling up quite a bit of solver changes in the PR queue and unfortunately many of them seem to be conflicting :(.

See also discussion in: #2928#issuecomment-157479392

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Nov 18, 2015

The reason is that @kosmikus seems to be busy with other things.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 18, 2015

AFAICT this is "new functionality" + "tests of said functionality". I don't understand the solver code, but the tests look pretty reasonable and seem like they cover at least the basic functionality...?

Of course, if @kosmikus thinks the tests aren't comprehensive enough, then that's perhaps worth blocking this on.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 18, 2015

I mean, if we're getting nervous about merging PRs with tests (with some semblance of coverage), then I'm going to get really worried :).

(Unless of course we're talking about things that are clearly out-of-scope for the project, but that's another matter.)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Nov 18, 2015

@kosmikus has the final word on everything related to the solver, and since he assigned this PR to himself, it's up to him to review and merge it.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 18, 2015

Fair enough, I guess.

I wonder if it would be a good idea to make "ownership" explicit, per http://meowni.ca/posts/chromium-owners/ ?

(I certainly know that I have no idea about who "owns" what bit of Cabal/cabal-install. Maybe it would be a good idea to make it explicit via OWNER/MAINTAINER files or something similar? It would certainly help to be able to ping the right people when making "drive-by" contributions.)

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 25, 2015

@kosmikus Any idea when you'll be able to look at this?

(Btw, I realize that I can be a little bit of a "bull in a china shop", but it's getting quite frustrating to have to put everything solver-related on hold. I'm also worried about the bus factor of 1 on the solver :/ .)

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Nov 25, 2015

@BardurArantsson I can understand your frustration, and share it: I'm frustrated myself that I haven't managed to look at anything related to Cabal in the past few weeks ... I'm also sorry, because I really appreciate these contributions. For this one, I'm relatively sure this is all fine, because I've already looked at a previous version, so I guess you can go ahead and merge it without another review by me.

@jdnavarro

This comment has been minimized.

Copy link
Collaborator

jdnavarro commented Nov 25, 2015

Actually the trickiest part of my commit was extending the testing DSL. I'd have more peace of mind if I explicitly knew I didn't butcher @edsko code.

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented Nov 25, 2015

What wait? Who butchered my code?? 😡 :-)

@jdnavarro

This comment has been minimized.

Copy link
Collaborator

jdnavarro commented Nov 25, 2015

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented Nov 25, 2015

Ah, this is what we briefly discussed at the hackathon? Without studying it in great detail your changes look reasonable.

@jdnavarro

This comment has been minimized.

Copy link
Collaborator

jdnavarro commented Nov 25, 2015

@edsko, yes, this is what we discussed at the Hackathon but we didn't have time to wrap it up.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 25, 2015

@kosmikus : Don't feel too bad -- nobody has infinite amounts of time and we all have to prioritize. I wonder if there's anybody who might be suitable to be a "co-owner" of the solver subsystem just so that we can increase the bus factor and avoid pipeline stalls...?

Given that the concensus here seems to be that this looks good, I'd be happy to press the "Merge" button. @jdnavarro : Any chance you could rebase on top of current master? (Just to make the commit graph look a little nicer :).)

@torilmud-silvanus

This comment has been minimized.

Copy link

torilmud-silvanus commented Nov 25, 2015

Hi! @BardurArantsson I think you meant to tag @jdnavarro in the above comment :)

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 25, 2015

Doh! Yes, sorry about that :)

kosmikus and others added some commits Jul 23, 2015

Track language extensions and language flavours in the solver.
Every package now "depends" on all language extensions
(default-extensions and other-extensions) and language flavours
(default-language and other-languages) it declares in its cabal file.

During solving, we verify that the compiler we use actually
supports selected extensions and languages. This has to be done
during solving, because flag choices can influence the declared
extensions and languages being used.

There currently is no equivalent check performed on the generated
install plans. In general, cabal-install performs a sanity check
on the solver output, checking that the solver e.g. indeed includes
all the declared dependencies of a package. There is no such
double-checking for language extensions. This is not really
problematic, as all that this change does is to make the solver
more conservative rather than less. However, having a sanity check
available might ultimately be nice to have.
Add solver tests for language extensions and flavours
This also includes modifications to the solver testing DSL and the
testing functions.

This is necessary for merging PR #2732.

@jdnavarro jdnavarro force-pushed the jdnavarro:lang-extensions branch from 594335b to 1f40772 Nov 25, 2015

@jdnavarro

This comment has been minimized.

Copy link
Collaborator

jdnavarro commented Nov 25, 2015

@BardurArantsson There you go

BardurArantsson added a commit that referenced this pull request Nov 26, 2015

Merge pull request #2873 from jdnavarro/lang-extensions
Tests for solver support language extensions and language flavours

@BardurArantsson BardurArantsson merged commit c170de4 into haskell:master Nov 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Nov 26, 2015

Merged, thanks!

@phadej

This comment has been minimized.

Copy link
Collaborator

phadej commented Nov 26, 2015

👍

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