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

Detect extra-deps missing from indices #1858

Closed

Conversation

luigy
Copy link
Contributor

@luigy luigy commented Feb 29, 2016

Stack used to silently accept extra-deps from config or cli that were missing from indices.
It now errors out when they are not found
(will add package suggestions for typos in another PR)

fixes #1521

@luigy luigy changed the title Detect missing extra-deps missing from indices Detect extra-deps missing from indices Feb 29, 2016
* it should now detect typos commercialhaskell#1521
  (package suggestions will be added in another PR)
@luigy luigy force-pushed the detect-missing-extra-deps branch from 2a95c77 to 8144ae6 Compare February 29, 2016 17:44
@mgsloan
Copy link
Contributor

mgsloan commented Mar 1, 2016

Awesome, thanks!

Can these please get turned into a warning? Having it as an error like this is indeed more straightforward, however, having this as an error rules out the following use cases:

  • Using an environment that does not have all of the required packages. For example, if you usually run with a docker container that has proprietary packages, but would still like to be able to build some packages outside of the container.
  • If the stack.yaml is required to build tools / run scripts which populate the package DB with some more packages, satisfying the extra-deps.

This may be a bit tricky when they're also accompanied by build plan failures as in the example in #1521 . Ideally they'd come at the end of the output in that case, since it's probably the issue. I haven't looked at how the code would need to change to do that. Feel free to keep this as an exception constructor even if it's never thrown.

@luigy
Copy link
Contributor Author

luigy commented Mar 1, 2016

Oh, I see! yes, definitely this should be delayed till later.

This may be a bit tricky when they're also accompanied by build plan failures as in the example in #1521 . Ideally they'd come at the end of the output in that case, since it's probably the issue.

stack can have loadSourceMap return these as warnings and catch build plan failures in Stack.Build.build to display the messages in a better order. A change like this to loadSourceMap could also be used by a possible solution to #1752 #1722 that also depend, in a way, on loadSourceMap not throwing and holding off till wether the plan succeeds or not. Stack will have to make it clear that is doing this

Hmm should extra-deps coming through the cli still error out, though, to match stack's original behavior? and only warn on the ones coming from the config?

@luigy
Copy link
Contributor Author

luigy commented Mar 2, 2016

looks like something for #1745 could help with display order of warnings

@mgsloan
Copy link
Contributor

mgsloan commented Mar 2, 2016

Yep, I encourage you to work on #1745 if you want to! Particularly if it makes the implementation of this cleaner (it probably would).

I haven't thought about it thoroughly, but this could look like adding some IORef [Message] type to Env. runStackT would always output these messages (even on exception), so that they don't get lost.

There's no need to go and convert all the existing warnings and such to this system, that could be done incrementally. It might make sense to write something that just handles this particular usecase well, and generalize from there.

@mgsloan
Copy link
Contributor

mgsloan commented Apr 22, 2016

@luigy Hey! I'm doing a bit of a PR cleanup sweep, how's it going?

This change seems a bit related to #1839 . If it makes it easier, feel free to merge them into one PR (potentially a new PR if you like)

@mgsloan
Copy link
Contributor

mgsloan commented Oct 24, 2016

@luigy I pondered how to implement this given all the various concerns, and it was indeed quite tricky! Turns out we needed to plumb the extraDeps info out of loadSourceMap and then check it against the installed packages. Here's the commit: ced6ea8

Thanks for working on this!

@mgsloan mgsloan closed this Oct 24, 2016
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.

Typos in extra-deps aren't detected
2 participants