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

Monorepo as Location logic breaks reformatting of spago.dhall #594

Closed
Profpatsch opened this issue Mar 13, 2020 · 9 comments
Closed

Monorepo as Location logic breaks reformatting of spago.dhall #594

Profpatsch opened this issue Mar 13, 2020 · 9 comments

Comments

@Profpatsch
Copy link

Profpatsch commented Mar 13, 2020

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 😛) 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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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?

@f-f
Copy link
Member

f-f commented Mar 16, 2020

@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 purescript/registry#4 will stay anywhere close to what it is now

@paulyoung
Copy link

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

@paulyoung
Copy link

I think this is also the cause of my problem here: #607 (comment)

@f-f
Copy link
Member

f-f commented Jul 22, 2020

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

@paulyoung
Copy link

When @Gabriel439 helped me to debug #607 (comment) I believe he had a concrete suggestion on an alternative way to do this but I can't recall what it was now.

jlavelle added a commit to mcneissue/purescript-snap that referenced this issue Oct 21, 2020
There is a spago bug that prevents a spago.dhall that has local dependents from being parsed correctly if it has additional attributes. See purescript/spago#594.
jlavelle added a commit to mcneissue/purescript-snap that referenced this issue Oct 21, 2020
There is a spago bug that prevents a spago.dhall that has local dependents from being parsed correctly if it has additional attributes. See purescript/spago#594.
@f-f
Copy link
Member

f-f commented Sep 20, 2023

The new Spago does not use Dhall anymore, closing

@f-f f-f closed this as completed Sep 20, 2023
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

No branches or pull requests

3 participants