Local dependency paths are incorrect when packages.dhall is in another directory

[elliotdavies]

I'm rolling out Spago in a monorepo where we have multiple packages, each with their own spago.dhall, and a single packages.dhall so that all the dependencies stay in sync.

The folder structure looks like:

- packages.dhall
- package-a
  - spago.dhall
- package-b
  - spago.dhall

The packages.dhall contains something like:

let additions = {
  package-a =
    mkPackage
      ./package-a/spago.dhall
      "./package-a"
      "v1.0.0"

, package-b =
    mkPackage
      ./package-b/spago.dhall
      "./package-b"
      "v1.0.0"
}

And then package-a lists package-b as a dependency in its spago.dhall.

package-b's path is listed as "./package-b" and this is read by Spago and passed straight to the compiler (with src/**/*.purs appended). However, because the packages.dhall has been moved up one level this doesn't resolve: the actual path to b from a should be ../package-b.

I put together an SSCCE here that you can run: https://github.com/elliotdavies/spago-dependencies-example

My proposed solution would be to modify Spago so that it checks the location of packages.dhall and generates the correct relative path. I'm happy to do that work if you think it's a good idea!

[f-f]

Hi @elliotdavies!

I am aware of this problem since a while, but I put off writing down an explanation until now - the reason is that I was not sure how much of an impact this had outside of my own usage, so I focused on other things. The first issue that uncovered this problem is #172 - by itself it's not a big deal to implement, but it's blocked until we fix this "path problem"

The problem is the following:

• it's possible to have relative imports in Dhall, so it comes kind of natural to build this kind of "monorepo-like" structures
• when reading the configuration in, all the relative imports are resolved when running Dhall, before in Haskell-land we get a Config containing a PackageSet.
  This means that if you have the following structure:

  .
  ├── foo
  │   ├── bar
  │   │   └── packages2.dhall    -- content: ../packages1.dhall
  │   └── packages1.dhall        -- content: ../packages.dhall
  └── packages.dhall             -- content: { repo = "../something" }
  

  and then you read in the packages2.dhall, the data delivered to the application will be the content of packages.dhall with no trace of any of the files that have been resolved along the way.
• So the TLDR is that the problem lies in how Dhall imports are resolved separately from the local paths in packages (which are just strings), and that we cannot get back that information (easily at least. We might be able to recover the import stack from Dhall, but I'm not sure)

A possible solution for this would be to:

• search in unevaluated Dhall AST for relative imports to packages.dhall (note: we'd have to adopt the convention that package-set-files can only be called like this, but it's fine), and compute the canonical path to the "bottom one" (i.e. the one that doesn't contain such imports, at which we stop. It's ok to do this blindly because Dhall forbids cyclic imports anyways)
• after we have this path, we can append it to local repos paths, hopefully recovering the correct path

Does this make sense to you?

Implementing this would require mostly working with unevaluated Dhall AST (as opposed to evaluated one which gets just read into Haskell types), which we already do quite a bit, e.g. take a look at the Config and in the PackageSet modules.
I'd be happy to help you out with the implementation if you wish, feel free to ask anything in here 🙂

[jmackie]

Is the problem not that the second argument to mkPackage is repo : Text, and that stringy path isn't being resolved correctly? (I might be wrong, in which case ignore me...)

My first thought was that the Local repo path should be made absolute in the context of the file calling the function? I.e. somewhere around: https://github.com/spacchetti/spago/blob/2b1c83343fd5be021879ba2cc049e98616d07041/app/Spago/PackageSet.hs#L63-L66

[f-f]

@jmackie the problem is indeed that the path is stringy, but also that it refers to the folder the packages.dhallcontaining it is in, but not the one from which the spago.dhall is invoked. Makes sense?

[jmackie]

Nah not really, but you got this so I'll pipe down 💪

[f-f]

@jmackie I think if you run @elliotdavies repro it might get clearer: basically the paths that you list in the packages.dhall are stringy and the application gets them as you typed them, regardless of where you run spago and how the Dhall relative imports get resolved - that is: the bug comes from the fact that Dhall's import resolution is totally independent from the "path" of a package declared in packages.dhall

So e.g. if you had a spago.dhall in the root folder then it would be fine because the paths still make sense relative to that directory, but it doesn't work when you try to run it from e.g. package-a, because the paths are not changed and they don't make sense from that location

[elliotdavies]

@f-f Just realised I never replied to this - thanks for the advice, I will have a go at fixing this when I next get the chance!

[Dretch]

There are some new Dhall features in the pipeline that might help with this - see: dhall-lang/dhall-lang#71

[f-f]

@Dretch great find! I even commented on that issue, but I totally forgot about it 😄

That looks exactly what we'd need in this case. More specifically we'd want Dhall to resolve relative paths to absolute ones based on where the imported file is, not on what the CWD is, and this is described in the issue you linked.

I'll take a look at standardizing it, as a lot of the work for it is already in place

[f-f]

Update:

• I opened a PR for standardizing a relative-paths-resolving behaviour in Dhall itself, in Implement importing paths as Location dhall-lang/dhall-lang#585
• That alone however is not enough to solve this, because if that PR goes through we'll need change in the format that package-sets uses for the upstream packages.dhall: the type of the repo field in the Package will need to change from Text to < Local : Text | Remote : Text | Environment : Text | Missing >.
  This can be done in backwards-compatible way (by introducing a packagesV2.dhall) so this should be fine

[f-f]

Update: the standardization in dhall-lang/dhall-lang#585 has been merged, and I opened a PR to implement it in dhall-lang/dhall-haskell#1019

After that's merged we'll need to wait for a new release, but we can start the migration in package-sets already

[elliotdavies]

@f-f Awesome, I've been following your progress 😀 Thanks for working on it!

[f-f]

Update: turns out we might not need to change package-sets, #301 should be enough 🎉

(@elliotdavies you can probably test that branch already and see if it works for you - please read through the changes first, as it might be broken/dangerous - I just tested it on our monorepo and it seems to work: KSF-Media/affresco#136)
(also cc @reactormonk that had a similar problem IIRC)