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

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

Closed
elliotdavies opened this issue Jun 3, 2019 · 12 comments

Comments

@elliotdavies
Copy link
Contributor

commented Jun 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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 🙂

@f-f f-f added the defect label Jun 3, 2019

@jmackie

This comment has been minimized.

Copy link

commented Jun 4, 2019

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:

-- We consider a "Remote" anything that `parseURI` thinks is a URI
makeRepo repo = case parseURI $ Text.unpack repo of
Just _uri -> Remote repo
Nothing -> Local repo

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@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

This comment has been minimized.

Copy link

commented Jun 4, 2019

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

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@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!

@f-f f-f added this to the 1.0 milestone Jun 8, 2019

@Dretch

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2019

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

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Update:

  • I opened a PR for standardizing a relative-paths-resolving behaviour in Dhall itself, in 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

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

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

@f-f f-f referenced this issue Jul 4, 2019

Merged

Import local packages `as Location` #301

10 of 10 tasks complete
@f-f

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

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)

@f-f f-f closed this in #301 Jul 29, 2019

f-f added a commit that referenced this issue Jul 29, 2019

Import local packages `as Location` (#301)
This changes how local packages are specified in the `packages.dhall`
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 #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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.