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

Add dhall support #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add dhall support #61

wants to merge 1 commit into from

Conversation

nmattia
Copy link
Owner

@nmattia nmattia commented Aug 26, 2018

Fixes #51

@philderbeast this adds support for reading package.dhall files (I still need to update the command line arguments and the documentation, but the meat is there). However I doubt that this is going to work if other files are imported from package.dhall, like this:

{ foo = ./some/path.dhall }

I'm not sure how this can be done in a way that works in the sandbox (dhall-nix suffers from the same problem). @Gabriel439 do you have thoughts on this?

@Gabriella439
Copy link

@nmattia: I think the issue is that you're reading the original file as text using builtins.readFile then writing it out again using writeText before passing it to dhall-to-json. If you passed the original file directly to dhall-to-json then relative paths would work.

Another approach is that you can normalize the original file using dhall which will generate a self-contained expression that you can then use inside any sandbox

@nmattia
Copy link
Owner Author

nmattia commented Aug 26, 2018

@Gabriel439 The problem here is that I don't want to pull the whole source tree into the nix store; ideally I'd copy only package.dhall and potentially the other files it depends on. This may be tricky to implement, unless I can ask dhall to give me all the dependencies that a particular file has on other file. Is that possible? E.g.:

-- baz.dhall
{ foo = ../bar/baz.dhall,
  quux = [ ./foo.dhall ]
}

and now I could call dhall-magic:

$ dhall-magic baz.dhall
../bar/baz.dhall
./foo.dhall

Using that I can call builtins.dirOf on baz.dhall and also get the files it depends on as nix paths.

@Gabriella439
Copy link

@nmattia: You can't yet query dhall for the dependencies, but I can add support for that

@Gabriella439
Copy link

@nmattia: Thinking about this more, I don't think that querying Dhall for dependencies would solve your problem. If you were able to query Dhall for the dependencies you'd also be able to directly interpret and normalize the expression using the dhall command and eliminate all the dependencies.

@philderbeast
Copy link
Contributor

Testing this pull request with siggy-chardust, I find it fails to build looking for an import from package.dhall;

    let defs = ./defaults.dhall

in    defs
    ⫽ { name =
          "siggy-chardust"
...
      }
flare-timing> snack build --package-yaml=siggy-chardust/package.dhall
...
↳ /nix/store/r7s863s0pl794xq0zmn54ldk35q78xgm-d2j
  ↳ /nix/store/defaults.dhall

Error: Missing file /nix/store/defaults.dhall
...

@Gabriella439
Copy link

@philderbeast: That looks like a broken package description because ./defaults.dhall is missing from that project

@philderbeast
Copy link
Contributor

philderbeast commented Oct 11, 2018

@Gabriel439 ./defaults.dhall is in the repository root. I run hpack-dhall from the root;

https://github.com/BlockScope/flare-timing/blob/master/build/library/Pkg.hs#L57

flare-timing> __shake-build/hpack-dhall siggy-chardust
siggy-chardust/siggy-chardust.cabal is up-to-date
                                                                                                                                                                               
flare-timing> rm siggy-chardust/siggy-chardust.cabal
                                                                                                                                                                               
flare-timing> __shake-build/hpack-dhall siggy-chardust
generated siggy-chardust/siggy-chardust.cabal

Running dhall from the root picks up ./defaults.dhall whereas running from the package's folder fails;

flare-timing> __shake-build/dhall < siggy-chardust/package.dhall
{ name :
    Text
, version :
    Text
...
, dependencies =
    [ "base >=4.9.1.0" ]
}

flare-timing> cd siggy-chardust/
siggy-chardust> ../__shake-build/dhall < package.dhall

↳ ./defaults.dhall
Error: Missing file /.../flare-timing/siggy-chardust/defaults.dhall

I will investigate changing to the package folder for the hpack-dhall call and using ../defaults.dhall instead as the import from package.dhall.

@Gabriella439
Copy link

@philderbeast: This seems like something that should be fixed in hpack-dhall rather than snack

Specifically, when hpack imports package.dhall it should do in such a way that any import in package.dhall is referenced relative to the package.dhall directory and not relative to the directory where hpack is run. That would entail a change to siggy-chardust/package.dhall to reference the defaults as ../defaults.dhall.

See this for more context: https://github.com/dhall-lang/dhall-lang/wiki/Frequently-Asked-Questions-(FAQ)#imports-relative-to-my-top-level-file-are-not-working

In general, Dhall files are not meant to be read directly via stdin because then import resolution is done relative to the current directory rather than relative to the file. For example, if you do:

__shake-build/dhall <<< './siggy-chardust/package.dhall'

... then you will get a failure (and it should fail).

@philderbeast
Copy link
Contributor

I think the issue is that you're reading the original file as text using builtins.readFile then writing it out again using writeText before passing it to dhall-to-json. If you passed the original file directly to dhall-to-json then relative paths would work.

Here's a concrete example of the above using the golden tests I added for cabalism/hpack-dhall#4 where empty-package.dhall is self-contained but import-local/package.dhall has a relative import;

hpack-dhall> stack install dhall dhall-json --stack-yaml=stack-8.2.2.yaml
...
Copied executables to /.../__bin:
- dhall
- dhall-to-json
- dhall-to-yaml

hpack-dhall> __bin/dhall-to-json < test/golden/hpack-dhall-cabal/empty-package.dhall
{"name":"empty-package"}

hpack-dhall> __bin/dhall-to-json < test/golden/hpack-dhall-cabal/import-local/package.dhall

↳ ./name.dhl

Error: Missing file /.../hpack-dhall/name.dhl

hpack-dhall> cd test/golden/hpack-dhall-cabal/import-local/

h/t/g/h/import-local> ../../../../__bin/dhall-to-json < package.dhall
{"name":"import-local"}

Similarly using dhall;

hpack-dhall> __bin/dhall < test/golden/hpack-dhall-cabal/import-local/package.dhall

↳ ./name.dhl

Error: Missing file /.../hpack-dhall/name.dhl

hpack-dhall> cd test/golden/hpack-dhall-cabal/import-local/

h/t/g/h/import-local> ../../../../__bin/dhall < package.dhall
{ name = "import-local" }

@Gabriella439
Copy link

@philderbeast: See my previous comment and in particular see this link: https://github.com/dhall-lang/dhall-lang/wiki/Frequently-Asked-Questions-(FAQ)#imports-relative-to-my-top-level-file-are-not-working. The issue is feeding the contents of each file directly to dhall's standard input

@philderbeast
Copy link
Contributor

philderbeast commented Oct 29, 2018

I've just published hpack-dhall-0.4.0. Could the executable dhall-hpack-json that resolves imports relative to package.dhall be used here instead of dhall-to-json?

"dhall-to-json <<< ${writeText "d2j" text} > $out"

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

Successfully merging this pull request may close these issues.

3 participants