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

Show resolver being used when stack ghci is invoked outside of a project directory #4521

Merged
merged 13 commits into from
Feb 10, 2019

Conversation

wafelj
Copy link
Contributor

@wafelj wafelj commented Jan 20, 2019

Fix for #3651.

Approach: read Project from Config, get SnapshotLocation and compiler info from it and use that to get resolver using the loadResolver function. To my understanding it is theoretically possible that there is no project information in config (it's wrapped in Maybe). But I'm assuming this never actually happens because if I try to delete the ~/.stack/global-project/stack.yaml, a new one is recreated when stack ghci is run again. This is why I assumed it's safe to use fromJust. If there is a better approach, I'd love to hear about it.

Disclaimer: I'm new to Haskell and to stack, and this is my first open-source contribution. Any feedback is most appreciated!

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

src/Stack/Ghci.hs Outdated Show resolved Hide resolved
src/Stack/Ghci.hs Outdated Show resolved Hide resolved
when (null localTargets && isNothing mfileTargets) $

when (null localTargets && isNothing mfileTargets) $ do
let project = fst $ fromJust mproject
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if there is no currently known way for this to be Nothing, there could possibly be a way now or in the future, and the error thrown by fromJust is pretty frustratingly vague.

How about instead, for the Nothing case, using something like error "Invariant violated: no project". One nice thing about using error is that it will show where in stack's code the error occurred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, ignoring the Nothing case is pretty ugly. I was thinking about this some more and decided to try something else: simply not print the resolver version if the project is Nothing. Would this work? I can do the error instead, but it's basically the same thing I had before, except it has a more useful error message.

@mgsloan
Copy link
Contributor

mgsloan commented Jan 20, 2019

The code related to resolvers and snapshots has changed quite a bit since I last looked, so I can't give direct guidance. However, it seems like loadResolver does fetching and parsing that I believe should have already happened. So, I think it'd be nice to avoid the redundant call, or cache it. I'll leave that decision up to the more active maintainers, though.

@wafelj
Copy link
Contributor Author

wafelj commented Jan 21, 2019

@mgsloan thanks for your review! I was digging through the HasEnvConfig environment, trying to see if and where SnapshotDefinition is available, but I could not find it. SnapshotLocation inside Project inside Config was the closest, so I went with that. Definitely interested in other suggestions - my understanding of the project is not very deep yet.

@mgsloan
Copy link
Contributor

mgsloan commented Jan 22, 2019

@wafelj Welcome! Yeah I also dug around HasEnvConfig looking for snapshot name info. Couldn't seem to find it, which seems surprising since EnvConfig has BuildConfig, and info about the snapshot needs to be loaded to have a BuildConfig. So, yeah, not sure why that's not there ¯_(ツ)_/¯

Looks to me like bcSnapshot :: LoadedSnapshot could simply be added to BuildConfig and then populated in loadbuildConfig.

@mihaimaruseac mihaimaruseac changed the title Fix #3651 - Show resolver being used when stack ghci is invoked outside of a project directory Show resolver being used when stack ghci is invoked outside of a project directory Jan 22, 2019
@qrilka
Copy link
Contributor

qrilka commented Jan 22, 2019

@mgsloan @wafelj there was a large refactoring in #4412 and now Stack doesn' use LoadedSnapshot anymore. I think snapshot name could be added into SMWanted which later gets transformed into SourceMap - snapshot gets loaded in Stack.Config.loadBuildConfig but its name gets ignored (only compiler/dependencies get used).

@wafelj
Copy link
Contributor Author

wafelj commented Jan 23, 2019

@mgsloan @qrilka thanks. I looked into that and changed the code - now it passes the name from SMWanted to SMActual to SourceMap. Then I load SourceMap from EnvConfig and read the name from that. Is this what you had in mind?

Also, two questions:

  1. SMWanted is available in in BuildConfig. Would it make sense to read it directly from that in targetWarnings function instead of passing it through all those other types?
  2. naming - is smwName etc. descriptive enough or would smwResolverName or smwSnapshotName etc. be better?

@qrilka
Copy link
Contributor

qrilka commented Jan 24, 2019

  1. The name should be the same in either of SMXXX or SourceMap so using BuildConfig should be OK
  2. I think we should use longer field name as it's not a name of a source map itself. Also it makes sense to follow Make snapshot: a synonym for resolver: #4256 and use "snapshot".
    @snoyberg should we also switch to use "snapshot" instead of "resolver" in Stack output?

@snoyberg
Copy link
Contributor

should we also switch to use "snapshot" instead of "resolver" in Stack output?

Sounds like a good idea to me.

@wafelj
Copy link
Contributor Author

wafelj commented Jan 24, 2019

@qrilka changed to read name directly from BuildConfig and changed the field to smwSnapshotName. Also changed output to snapshot instead of resolver. Looks good?

@dbaynard dbaynard merged commit 7a076ab into commercialhaskell:master Feb 10, 2019
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.

5 participants