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

ob run: Query local packages from Nix project #489

Merged
merged 31 commits into from
Jan 24, 2020
Merged

ob run: Query local packages from Nix project #489

merged 31 commits into from
Jan 24, 2020

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Aug 4, 2019

This enhances the getLocalPkgs function so that packages defined in the Nix project get added to the GHCi config.

This PR is slightly related to #475. The readProcessJSONAndLogStderr function could also be used there.

@rvl
Copy link
Contributor Author

rvl commented Aug 7, 2019

Thanks, bit busy for the next few days, but I will fix the typo noted. Also I realised that the extra local packages must be added to shells.ghc and shells.ghcjs, so will push a change to default.nix. And I will also upload my project which uses this feature.

@rvl
Copy link
Contributor Author

rvl commented Aug 10, 2019

Thanks for the reviews. I have fixed everything and rebased.

@3noch
Copy link
Collaborator

3noch commented Sep 4, 2019

I'm suspicious of these changes for a reason I've forgotten. Need to go back and remember....

@3noch
Copy link
Collaborator

3noch commented Sep 4, 2019

Oh now I remember. Pulling in all of the packages in packages is likely going to have bad effects for many obelisk projects. GHCI can only load one set of extensions at once and if any of the modules disagree then they will fail to load IIRC. If we're going to do this we probably need to use a separate attribute, distinct from packages that lets people specify exactly what they want in-scope during ob repl etc.

@alexfmpe
Copy link
Contributor

alexfmpe commented Sep 4, 2019

I've talked about this sort of thing before with @ryantrinkle, who suggested having only unpacked thunks from packages be thrown into GHCI.

@alexfmpe
Copy link
Contributor

alexfmpe commented Sep 4, 2019

I think there's also some progress on adding actual multi package support to GHCI.
@Ericson2314 ?

@3noch
Copy link
Collaborator

3noch commented Sep 4, 2019

Even restricting to unpacked thunks seems too onerous IMO. The problem will surface for a package regardless of how it's retrieved so you will always want to be able to specify which packages get this extra treatment.

@alexfmpe
Copy link
Contributor

alexfmpe commented Sep 4, 2019

The way I see it, the point of having a thunk be unpacked is editing it, in which case you'd want it thrown in more often than not.
Though I don't see a problem with some ghciPackages option that'd default to (unpacked or not) packages

@3noch
Copy link
Collaborator

3noch commented Sep 4, 2019

Yes the point is editing, but you definitely need a way to make it not get loaded into GHCi or unpacking might break ob run...

@Ericson2314
Copy link
Member

I might like see a 3-way "definitely build", "definitely ghci", and "do automatically based in unpackedness"

@ryantrinkle
Copy link
Member

Regarding "one set of extensions" and such: what if we inject a preprocessor (-pgmL or similar) that injects LANGUAGE pragmas and/or OPTIONS_GHC pragmas at the top of each file. Is there anything we couldn't do that way?

@3noch
Copy link
Collaborator

3noch commented Jan 10, 2020

That's a very interesting idea.

@3noch
Copy link
Collaborator

3noch commented Jan 13, 2020

We're going to try this out via preprocessor and discriminating on packedness of dependencies.

@rvl
Copy link
Contributor Author

rvl commented Jan 14, 2020

The reason that I made this PR is that, in addition to the standard 3 local packages (frontend, backend, common), I had an additional mypackage, that I needed to be loaded into ob repl. My simplifying assumption was that mypackage would share the same flags and default extensions as the other local packages. Additional non-local package dependencies would still go into the project's haskellPackages overrides as before.

@3noch
Copy link
Collaborator

3noch commented Jan 14, 2020

@rvl A lot of people have been rooting for this PR. We really appreciate your work! Sorry it's taken so long to get attention. You're right that on average the cabal settings for multiple packages will probably work well together. The difficulty has been: "How do we get this to a point where we can confidently release it to everyone by default?" I think the preprocessor idea finally unlocks that piece of the puzzle because we can actually load each module the way it was meant to be loaded. Do you foresee issues with this plan?

@rvl
Copy link
Contributor Author

rvl commented Jan 14, 2020

The preprocessor idea sounds like more work to implement but the end result will be better for users. It will also help in the case where users have (for example) modified default extensions in frontend.cabal but not in the other two cabal files.

@3noch 3noch mentioned this pull request Jan 16, 2020
4 tasks
@3noch 3noch removed the External label Jan 16, 2020
@rvl
Copy link
Contributor Author

rvl commented Jan 17, 2020

@3noch It's looking good, thanks!

@3noch 3noch merged commit 796fac5 into obsidiansystems:qa Jan 24, 2020
@3noch
Copy link
Collaborator

3noch commented Jan 24, 2020

@rvl This is now in the qa branch to undergo more rigorous testing. Please report any issues you find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants