Import local packages as Location

[f-f]

This PR fixes #244, by changing how local packages are specified in the packages.dhall (yes this is a breaking change, because the old local syntax will now error out)

Taking the README example, we go from this:

let additions =
  { foobar =
      mkPackage
        (../foobar/spago.dhall).dependencies
        "../foobar"
        "local-fix-whatever"
  }

to this:

let additions = 
  { foobar = ../foobar/spago.dhall as Location }

This means a couple of things:

• the issue from Local dependency paths are incorrect when packages.dhall is in another directory #244 is fixed because Dhall will adjust the relative path properly
• but since it won't resolve the file, it's now possible to have a single shared packages.dhall, that will contain e.g. all the packages in your repo (this wasn't possible before as you'd have circular imports in your Dhall file)

Things missing:

• handle all the various errors with nice error messages
• wait for a new dhall-lang release, happening in Version 8.0.0 → 9.0.0 dhall-lang/dhall-lang#635
• wait for a new dhall-haskell release, happening in Version 1.24.0 → 1.25.0 dhall-lang/dhall-haskell#1156
• wait for Add bump version command that creates a git tag and a bower.json file (issue #203) #289 to be merged
• add tests for building with local packages
• add tests for the list-packages command
• update the README
• add a proper "monorepo" section
• verify that Watch all sources, including dependencies, when building #309 works properly
• check for references of mkPackage (see Remove mkPackage Dhall function and switch to package-sets releases #322)

[elliotdavies]

@f-f Thanks for this, I'll try it with our monorepo. Excited that it also properly supports a single packages.dhall - we've been factoring out spago-deps.dhall files to avoid the circular imports and it's a pain!

[elliotdavies]

@f-f Found an interesting bug (I think) with circular imports. Given this setup, which is roughly what we have at Habito:

- lib
  - purescript
    - packages.dhall
    - lib1
      - spago.dhall
    - lib2
      - spago.dhall
app1
  - spago.dhall

Where packages.dhall is our shared single packages file and imports the libs like:

let additions =
  { lib1 = ./lib1/spago.dhall as Location
  , lib2 = ./lib2/spago.dhall as Location
  }

And each lib has a pretty trivial spago.dhall that includes packages = ../packages.dhall, and similarly app1/spago.dhall includes packages = ../lib/purescript/packages.dhall.

If I list all my dependencies inline as usual, spago install works just fine (:tada:). If, say, lib1 has its dependencies in another file (like dependencies = ./spago-deps.dhall where the deps file just exports an array of strings) then spago install will report

spago: The following packages have circular dependencies:
  - lib1

I don't think this is to be expected since the spago-deps.dhall file is a leaf node; the actual cycle is between packages.dhall and lib1/spago.dhall. It's not necessarily a blocker to merging this PR but it is weird.

[f-f]

@elliotdavies wow great catch! I can reproduce the issue locally, and it's not due to circular dependencies per se, but it's because the imports in lib1/spago.dhall are resolved relatively to the folder you're calling Spago from.
So e.g. if you are in app1 and you have a deps.dhall both in lib1 and in app1, you'll get the latter one, which most likely contains lib1 as a dependency, causing the circular deps problem.

So the root cause is the kinda-hacky implementation (but I cannot think of many other possible ones, so we'll keep this one) of local packages resolution here:

https://github.com/spacchetti/spago/blob/cfa28d7142cc45a7415dde7f1faaa68b51e3c3c7/src/Spago/Config.hs#L76-86

What's happening here is that we:

• pattern match on a as Location import in line 76
• read the raw AST (i.e. not resolved yet, so it contains imports and everything) in line 81
• transform the AST so that it goes from a config like { packages = .., dependencies = .. } to just whatever the content of dependencies is, in line 85
• then resolve the expression in line 86 (relative to our current directory)

So if the expression is pure no problem, if it contains relative imports then they are not going to be correct.

Two possible solutions here:

• the quick one is that we just forbid imports inside dependencies for local packages
• the right one (but the one with more potential corner cases) is to just set the relative path to the imported package

[f-f]

Right, I recalled there was a nice way to plug this in, so it was an easier fix than I expected.
b12ea08 fixes the issue for me locally, @elliotdavies could you try it?

[elliotdavies]

@f-f Yep, that's done it! (After a brief moment of panic when Spago just said "aaa" - eventually I realised I'd hit this TODO error case 😂) Glad you were able to find a nice fix.

I might have some time Sunday or Monday if you'd like a hand ticking off some of the TODO items for this?

[f-f]

@elliotdavies thanks, some help would be lovely! There are in fact some independent changes that should be merged in master before this goes in, and that you could pick up:

• adding tests with output fixtures for spago list-packages and spago list-packages --json (since I got very close to breaking them in this PR, so I'd like to have tests merged before to ensure I'm not breaking their behaviour) EDIT: I did it in Add tests for the list-packages command #304
• adding tests for spago build involving local packages (we don't have any) and your current monorepo setup (it'd be a nice demonstration of this feature to go from having to split out the dependencies in separate files to just having them in the spago.dhall)

[f-f]

cc @Dretch: since in this branch I'm shuffling code for the Config parsing (mainly going from Either to IO as the base monad to make things less wrapped, since we error out immediately anyways) in bf71f81 I'm also absorbing the PublishConfig into the Config to have less parsing code

[Dretch]

cc @Dretch: since in this branch I'm shuffling code for the Config parsing (mainly going from Either to IO as the base monad to make things less wrapped, since we error out immediately anyways) in bf71f81 I'm also absorbing the PublishConfig into the Config to have less parsing code

It all looks sensible as far as I can see. 👍