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

Import local packages `as Location` #301

Merged
merged 33 commits into from Jul 29, 2019

Conversation

@f-f
Copy link
Member

commented Jul 4, 2019

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 #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 dhall-lang/dhall-lang#635
  • wait for a new dhall-haskell release, happening in dhall-lang/dhall-haskell#1156
  • wait for #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 #309 works properly
  • check for references of mkPackage (see #322)
@elliotdavies

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@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 (🎉). 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

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

@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 #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 f-f referenced this pull request Jul 7, 2019

Merged

Add `--no-config-format` flag. Fixes #300. #302

2 of 3 tasks complete
@f-f

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

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

f-f added some commits Jul 8, 2019

@f-f f-f marked this pull request as ready for review Jul 8, 2019

@Dretch

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

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. 👍

f-f added some commits Jul 9, 2019

@f-f f-f referenced this pull request Jul 10, 2019

Closed

Watch directory dependencies? #172

f-f added some commits Jul 10, 2019

f-f added some commits Jul 14, 2019

@f-f f-f added the blocked label Jul 16, 2019

f-f added some commits Jul 17, 2019

@f-f f-f referenced this pull request Jul 21, 2019

Merged

Migration from Bower #272

0 of 3 tasks complete

f-f added some commits Jul 22, 2019

@f-f f-f changed the title Import local packages with location Import local packages `as Location` Jul 22, 2019

@f-f f-f referenced this pull request Jul 22, 2019

Open

Specify config path #329

f-f added some commits Jul 24, 2019

f-f added some commits Jul 27, 2019

@f-f f-f removed the blocked label Jul 29, 2019

@f-f f-f merged commit e42df5c into master Jul 29, 2019

3 checks passed

Summary 1 rule matches and 1 potential rule
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@mergify mergify bot deleted the local-packages-with-location branch Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.