spago icon indicating copy to clipboard operation
spago copied to clipboard

Monorepo `as Location` logic breaks reformatting of spago.dhall

Open Profpatsch opened this issue 5 years ago • 8 comments

We are using spago 0.14.0 from easy-purescript-nix: https://github.com/justinwoo/easy-purescript-nix/blob/14e7d85431e9f9838d7107d18cb79c7fa534f54e/spago.nix

When you have a monorepo as described in https://github.com/purescript/spago/blob/0.14.0/README.md#monorepo, you use something like

subpackage = ./subpackage/spago.dhall as Location

in packages.dhall to break the import recursion that would otherwise exist, because the subpackage/spago.dhall references ../packages.dhall again.

In our case, we (royal we :stuck_out_tongue:) cannot reference packages.dhall directly from the subpackage, because we need information like the package name and dependency list for our nix build. So the idea is to split the “data” into a different file

package.dhall: { name = "subpackage", dependencies = … }

and have spago.dhall

  (./package.dhall).{ sources, name, dependencies }
â«˝ { packages = ../packages.dhall }

which looks like it should work, it’s valid dhall after all. But suddenly, we got this confusing error message:

> spago build
spago: 
Error: Expression doesn't match annotation

- List …
+ { … : … }

1│   (./package.dhall).{ sources, name, dependencies }
2│ ⫽ { packages = ../packages.dhall } : List Text

(input):1:3

Where does this List Text type annotation come from‽

Turns out the code is doing some magic, ostensibly to avoid reparsing ../packages.dhall(?):

https://github.com/Profpatsch/spago/blob/7a99343e4876a465600eaa64b0697a9f0b2a49a9/src/Spago/Config.hs#L95-L104

where expr is created by just parsing the file, and then filtered to extract the dependencies field via

https://github.com/Profpatsch/spago/blob/7a99343e4876a465600eaa64b0697a9f0b2a49a9/src/Spago/Config.hs#L378-L387

The dependencies field expression is then typed with List Text and imported as a value.

This means that Spago currently implicitly assumes the expression structure of the spago.dhall file, and if you differ (by for example using dhall’s import capabilities), you will get strange error messages like the one above.


On further experimenting, a spago.dhall like

let foo =
      { sources = [] : List Text
      , name = "foo"
      , packages = {=}
      , dependencies = [] : List Text
      }

in  foo

seems to parse fine, so at least some normalizing is done somewhere.

Profpatsch avatar Mar 13 '20 00:03 Profpatsch

Update: it’s not the record projection, this example works:

let foo =
      { sources = [] : List Text
      , name = "foo"
      , dependencies = [] : List Text
      , packages = {=}
      }.{ sources, name, packages, dependencies }

in  foo

Profpatsch avatar Mar 13 '20 00:03 Profpatsch

This, however, fails again:

let foo =
        (./package.dhall).{ sources, name, dependencies }
      â«˝ { packages = ../packages.dhall }

in  foo
spago: 
Error: Expression doesn't match annotation

- List …
+ { … : … }

1│ let foo =
2│         (./package.dhall).{ sources, name, dependencies }
3│       ⫽ { packages = ../packages.dhall }
4│
5│ in  foo : List Text

(input):1:1

Maybe it’s a dhall library bug?

Profpatsch avatar Mar 13 '20 00:03 Profpatsch

Update: It works with the following workaround:

let pkg = ./package.dhall

in  { sources = pkg.sources
    , name = pkg.name
    , dependencies = pkg.dependencies
    , packages = ../packages.dhall
    }

I assume this works because we have a RecordLit here to destructure on?

Profpatsch avatar Mar 13 '20 00:03 Profpatsch

@Profpatsch yeah sorry for that, the way we treat the config file is fairly magick for a series of historical reasons.

I hope to solve all of this by switching the place where as Location happens: so we'd refer to local dependencies with normal imports, but import the Package Set as Location. We might be able to do this change in conjunction with the new registry, if the format in the PoC in https://github.com/purescript/registry/pull/4 will stay anywhere close to what it is now

f-f avatar Mar 16 '20 11:03 f-f

I just ran into this and found it to be very confusing.

paulyoung avatar Jul 03 '20 03:07 paulyoung

I think this is also the cause of my problem here: https://github.com/purescript/spago/issues/607#issuecomment-662232387

paulyoung avatar Jul 22 '20 19:07 paulyoung

This will likely be fixed properly once the new Registry is deployed, so most of my focus is going to be on moving that forward rather than trying to find a solution to this that works in the meanwhile. I am open for proposals/PRs here, but I have the feeling that fixing this will require a breaking change anyways

f-f avatar Jul 22 '20 20:07 f-f

When @Gabriel439 helped me to debug https://github.com/purescript/spago/issues/607#issuecomment-662232387 I believe he had a concrete suggestion on an alternative way to do this but I can't recall what it was now.

paulyoung avatar Aug 13 '20 20:08 paulyoung

The new Spago does not use Dhall anymore, closing

f-f avatar Sep 20 '23 16:09 f-f