Skip to content

Conversation

Lagoja
Copy link
Contributor

@Lagoja Lagoja commented Mar 16, 2023

Summary

This PR addresses #29, using a similar framework as our PHP planner. It makes it possible to use Haskell packages installed via haskellPackages alongside GHC.

This PR is incomplete - right now it only works for the default ghc in Nixpkg. We should make sure that we can detect when users request a different compiler version (like haskell.compiler.ghc810)

How was it tested?

Default GHC: Use the following devbox.json:

{
  "packages": [
    "haskellPackages.aeson",
    "ghc",
    "stack"
  ],
  "shell": {
    "init_hook": null
  },
  "nixpkgs": {
    "commit": "f80ac848e3d6f0c12c52758c0f25c10c97ca3b62"
  }
}

Run devbox shell, then run ghc-pkg list | grep aeson to make sure that it installed correctly

Versioned GHC: Use the following devbox.json

{
  "packages": [
    "haskell.compiler.ghc92",
    "haskell.packages.ghc92.aeson",
    "tree",
    "direnv",
    "ghc",
  ],
  "shell": {
    "init_hook": null
  },
  "nixpkgs": {
    "commit": "f80ac848e3d6f0c12c52758c0f25c10c97ca3b62"
  }
}

Run devbox shell, then run ghc-pkg list | grep aeson to make sure that it installed correctly. Also confirm that other packages installed as expected

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

nice!

Comment on lines +37 to +56
func (p *V2Planner) IsRelevantForPackages(packages []string) bool {
p.userPackages = packages
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of scope for this PR, but this is a strange API, where we save packages as a side-effect in a boolean IsRelevantForPackages function call....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also weird that we do 2 passes of the regex, one to detect if the planner is relevant, and another to actually create the shell plan -- should we be storing the results of the IsRelevantForPackages and then reusing it in GetShellPlan?

@Lagoja Lagoja force-pushed the jl/haskell-package-support branch from 452360c to 8799fce Compare March 22, 2023 00:10
@Lagoja Lagoja marked this pull request as ready for review March 22, 2023 00:11
@Lagoja
Copy link
Contributor Author

Lagoja commented Mar 22, 2023

This is now working for both the versioned and default GHC packages. My Go code may be a bit gnarly, so happy to make updates where needed

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

@Lagoja mostly lgtm, but what do you think of the union idea below?

Comment on lines -27 to -32
pkgs := p.GetShellPlan(srcDir).DevPackages
mutualPkgs := lo.Intersect(userPkgs, pkgs)
// Only apply shell plan if user packages list all the packages from shell plan.
if len(mutualPkgs) == len(pkgs) {
// if merge fails, we return no errors for now.
result, _ = plansdk.MergeShellPlans(result, p.GetShellPlan(srcDir))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is safe given our haskell and php planners. The getRelevantPlanners should filter appropriately.

Comment on lines 125 to 128
// If the DevPackages are empty, set them to userDefinedPkgs.
if len(shellPlan.DevPackages) == 0 {
shellPlan.DevPackages = userDefinedPkgs
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Lagoja I think we want a union of userDefinedPkgs and DevPackages. That way, each shell planner can be focussed purely on its own packages. And we don't drop other user packages.

WDYT?

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 think that might be safer, but it means that planners can't filter the package list after adding them to the definition, which leads to weird stuff like including PHP extensions and haskell compilers twice.

I'll test with union, and if everything works we can go that way for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to:

shellPlan.DevPackages = pkgslice.Unique(append(shellPlan.DevPackages, userDefinedPkgs...))

@Lagoja Lagoja merged commit dfcd319 into main Mar 23, 2023
@Lagoja Lagoja deleted the jl/haskell-package-support branch March 23, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

devbox add ghc && devbox add haskellPackages.aeson does not add aeson to ghc package database
2 participants