Adapt to Dhall version 1.19.0

jneira commented 5 years ago

I am not totally confidente about this although it typechecks so i guess it will be correct :wink:

jneira commented 5 years ago

cabal-to-dhall golden tests are failing for me in windows with:

SPDX:                     FAIL
  Exception: fd:4: hGetContents: invalid argument (invalid byte sequence)

I am investigating the cause

jneira commented 5 years ago

cabal-to-dhall < golden-tests\cabal-to-dhall\SPDX.cabalworks perfectly :thinking:

jneira commented 5 years ago

It is a issue with the diff external tool used by cabal-to-dhall golden tests. I've rebased the branch that uses internal diff and all tests pass,

jneira commented 5 years ago

To make the nix build work i would need the required change in dhall.nix with the correct hash

jneira commented 5 years ago

I've bumped up the version to 1.4.0.0 cause it seems to me that the new let grammar would be a breaking change

jneira commented 5 years ago

Ok, so the actual layout of prelude.dhall is not possible with the new direct union access cause we cant mix types and values in a record (see https://github.com/dhall-lang/dhall-haskell/issues/692#issuecomment-440970889). Luckily the constructors === id is going to be merged for the next dhall release so we can postpone the change (that i had tested in https://github.com/eta-lang/dhall-to-etlas/compare/dhall-master...eta-lang:union-access?expand=1)

jneira commented 5 years ago

I've managed to install cabal2nix in windows so i've able to make the derivation of dhall-1.19.0 :laughing: I hope hydra will be happy with it.

jneira commented 5 years ago

So cabal-to-dhall tests are failing in hydra with:

cabal-to-dhall
    simple:                   FAIL (0.02s)
      Test output was different from 'golden-tests/cabal-to-dhall/simple.dhall'. Output of ["diff","-u","golden-tests/cabal-to-dhall/simple.dhall","/tmp/nix-build-dhall-to-cabal-1.3.1.0.drv-0/simple.dhall16283-0.actual"]:
      --- golden-tests/cabal-to-dhall/simple.dhall      1970-01-01 00:00:01.000000000 +0000
      +++ /tmp/nix-build-dhall-to-cabal-1.3.1.0.drv-0/simple.dhall16283-0.actual        2018-11-22 12:22:13.502765537 +0000
      @@ -9,4 +9,4 @@
                 prelude.v "1.0"
             , cabal-version =
                 prelude.v "2.0"
      -      }
      +      }
      \ No newline at end of file

That means that dhall files need to have a newline? My tests using internal diff instead external tool are passing so maybe it would be time to add those changes?

quasicomputational commented 5 years ago

AIUI we don't need to block on constructors === id, right? Just removing prelude.types is fine, since we now have a better way of getting at the individual constructors in the language itself.

The test failures don't look serious - disagreements in the golden files about final newlines.

RE the one change you're uncertain about: it looks to me like it's being exercised well enough by the tests. Typechecker's happy, tests are happy, looks sensible enough to me, so I'd say go for it.

jneira commented 5 years ago

@quasicomputational yeah, the simplest solution was just in front of my eyes but i didnt see, thanks!

Gabriella439 commented 5 years ago

Also, for future reference, you're always welcome to add dhall-to-cabal to the dhall Haskell mega-project if you want dhall-to-cabal to be automatically fixed whenever the Haskell API makes a breaking change. This would ensure that the Haskell implementation can't merge a pull request that breaks dhall-to-cabal. Of course, you don't have to if you don't want to

jneira commented 5 years ago

Move the repo has advantages but some caveats too:

You have to fork and use as submodule the entire repo and it would suppose maintain new branches if you already forked the dhall-haskell one
- In my case i maintain a fork of dhall-to-cabal suitable for eta/etlas and we use it as submodule for etlas (and transitively for eta)
We have to take in account dhall-to-cabal in dhall-haskell releases (this is the point for move it but maybe it can delay dhall-haskell releases and affect other users not interested in dhall-to-cabal)
Conceptually i feel that dhall-to-cabal (as dhall-nix) doen't have the same scope that dhall-json or dhall-text (although it is more related with dhall-bash, that already is in the mega repo). Text and json are very common utilities and others are about specifical build tools.

However, if others maintainers (@ocharles and @quasicomputational) thinks is a good idea i will be totally on board with it :smile:

jneira commented 5 years ago

About the pr my plan is:

[X] Merge the internal diff tests branch in this one to make hydra build green and tests the subsequent changes
[X] Remove the use of constructors in the code (in this branch) cause it should improve dhall-to-cabal performance

However the pr could be merged as is. Let me know if you think it you think last one option should be done.

ocharles commented 5 years ago

I think we'll keep it separate for now. Thanks for all your work @jneira, I'll try and review this soon!

jneira commented 5 years ago

@ocharles I've already merged the branch union-access removing the use of constructors in the code base.

jneira commented 5 years ago

@ocharles i think it is ready to be merged (modulo the observations i've already wrote)

ocharles commented 5 years ago

Thanks for all your hardwork here @jneira - I should be able to get to this soon.

jneira commented 5 years ago

Hi @ocharles, dont want to bother but dhall is going forward pretty fast and we are behind two major versions now. Maybe another maintainer :wink: could do a review if you are busy?

Well i had missed a @quasicomputational comment review and it seems it is better to follow the proposal, so i'll try to implement it before merging

jneira commented 5 years ago

Superseded by #145

dhall-lang / dhall-to-cabal

Adapt to Dhall version 1.19.0 #143