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

WIP: move to Cabal 2.2 #42

Merged
merged 4 commits into from
Jun 3, 2018
Merged

Conversation

quasicomputational
Copy link
Collaborator

It compiles and dhall-to-cabal dhall-to-cabal.dhall works, but:

  • The code style isn't right. I'll fix that up.
  • It needs tests, particularly the SPDX stuff. Hopefully (some of) Adapt golden tests to last version of lib #35 will get into master soon.
  • ...but I'd like another pair of eyes on the SPDX stuff to make sure it's not been done in a stupid way. I cribbed it off how VersionRange is handled, and I've tried to make it sane and ergonomic.

@ocharles
Copy link
Member

ocharles commented May 9, 2018

Wow, this is quite a PR! Give me a bit of time to review it, but I'll take a look. Thanks!

@quasicomputational
Copy link
Collaborator Author

If it makes it less intimidating, by lines changed it's about 50% generated by --print-type and then lightly edited for formatting. There are just an awful lot of SPDX identifiers!

@quasicomputational
Copy link
Collaborator Author

I've fixed the code style, and there's a bare-bones test that ought to exercise all the SPDX code going both ways (and which indeed found some bugs). Thoughts particularly wanted on the Dhall-side API for SPDX things; have a look at golden-tests/dhall-to-cabal/SPDX.dhall for a synthetically-complicated example.

@ocharles
Copy link
Member

I haven't forgotten about this, but I'm really short on time atm. I will try and get to it soon!

@ocharles
Copy link
Member

I think this is gonna be good to go. Can you merge in master, and ideally #64? Might be easier to wait until that lands in master.

@quasicomputational
Copy link
Collaborator Author

I'll rebase once #64 lands.

@quasicomputational
Copy link
Collaborator Author

Actually, #64 shouldn't cause a big merging headache, so I'll go ahead and rebase now and see what the damage is.

quasicomputational added a commit to quasicomputational/dhall-to-cabal that referenced this pull request May 26, 2018
@ocharles
Copy link
Member

I'll push a commit that fixes the Nix stuff.

@quasicomputational
Copy link
Collaborator Author

I've just realised that all those SPDX licenses are going to blow up --print-type Package. I don't think that's a reason to block merging, but I think we'd better consider alternatives (e.g., referencing things by import by default, and only dumping the enormous self-contained type if it's specifically asked for).

@ocharles
Copy link
Member

@Gabriel439 The above build has failed because Hydra is out of disk space. You might want to enable automatic GC.

@Gabriella439
Copy link
Contributor

@ocharles: I do enable automatic GC. It's just that the machine only has 20 GB of disk space. I'm going to try to upgrade the disk space

quasicomputational added a commit to quasicomputational/dhall-to-cabal that referenced this pull request May 27, 2018
@quasicomputational
Copy link
Collaborator Author

quasicomputational commented May 27, 2018

This PR is severely damaging performance (factor of ~5 slowdown over HEAD running the tests), which I assume is because of the enormous types/SPDX/LicenseId.dhall enumeration.

On master:

$ cabal new-test -w ghc-8.2.1
Build profile: -w ghc-8.2.1 -O1
In order, the following will be built (use -v for more details):
 - dhall-to-cabal-1.0.0.1 (test:golden-tests) (first run)
Preprocessing test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Building test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Running 1 test suites...
Test suite golden-tests: RUNNING...
golden tests
  dhall-to-cabal
    dhall-to-cabal:           OK (12.51s)
    gh-55:                    OK (5.04s)
    gh-53:                    OK (4.31s)
    nested-conditions:        OK (5.74s)
    empty-package:            OK (1.18s)
    conditional-dependencies: OK (6.43s)
    compiler-options-order:   OK (6.43s)
  cabal-to-dhall
    conditional-dependencies: OK
    gh-36:                    OK (0.01s)
    simple:                   OK

All 10 tests passed (41.66s)

On this branch:

$ cabal new-test -w ghc-8.2.1
Build profile: -w ghc-8.2.1 -O1
In order, the following will be built (use -v for more details):
 - dhall-to-cabal-1.0.0.1 (test:golden-tests) (first run)
Preprocessing test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Building test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Running 1 test suites...
Test suite golden-tests: RUNNING...
golden tests
  dhall-to-cabal
    dhall-to-cabal:           OK (65.59s)
    gh-55:                    OK (14.73s)
    SPDX:                     OK (23.23s)
    gh-53:                    OK (16.26s)
    nested-conditions:        OK (25.48s)
    empty-package:            OK (1.65s)
    conditional-dependencies: OK (25.25s)
    compiler-options-order:   OK (20.65s)
  cabal-to-dhall
    conditional-dependencies: OK
    gh-36:                    OK (0.02s)
    simple:                   OK
    SPDX:                     OK

All 12 tests passed (192.87s)

@quasicomputational
Copy link
Collaborator Author

I have been investigating the performance woes, and it's Dhall's typechecking that's taking 90% of the time when I convert dhall-to-cabal.dhall. The prelude.types.LicenseId record is pretty big; using constructors on a union is O(n^2) on the number of constructors in the union, and LicenseId has over 300. It seems like that and the big enum itself are getting re-typechecked many, many, many times, which isn't helpful.

My current theory is that this is mostly due to how, when the prelude variable is referenced, as well as prelude.types, those whole records are getting re-typechecked. I don't know if that's the whole story, but I'm going to construct a test to see if it could be.

I've also got a tiny little patch that's an immediate 40% win, but it definitely doesn't solve the problem and the performance is still totally unacceptable.

@quasicomputational
Copy link
Collaborator Author

I've opened dhall-lang/dhall-haskell#412 about the performance issue. Looks like it might be entirely a dhall-haskell thing, and solvable there.

@ocharles
Copy link
Member

I think either way, it'd be good to get this in as it's a pretty huge branch. Could you get the conflicts resolved?

quasicomputational and others added 2 commits May 30, 2018 09:11
The biggest change is the introduction of SPDX licenses, which have to
be reflected in Dhall; about half of the lines added are just license
identifiers.
@quasicomputational
Copy link
Collaborator Author

Rebased and squashed down.

@quasicomputational
Copy link
Collaborator Author

quasicomputational commented Jun 2, 2018

Here's the test suite's output with dhall-lang/dhall-haskell#420 applied on top of dhall-haskell HEAD:

$ cabal new-test -w ghc-8.2.1
Build profile: -w ghc-8.2.1 -O1
In order, the following will be built (use -v for more details):
 - dhall-to-cabal-1.0.0.1 (test:golden-tests) (first run)
Preprocessing test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Building test suite 'golden-tests' for dhall-to-cabal-1.0.0.1..
Running 1 test suites...
Test suite golden-tests: RUNNING...
golden tests
  dhall-to-cabal
    dhall-to-cabal:           OK (10.73s)
    gh-55:                    OK (4.37s)
    SPDX:                     OK (5.15s)
    gh-53:                    OK (4.04s)
    nested-conditions:        OK (6.03s)
    empty-package:            OK (0.66s)
    conditional-dependencies: OK (5.99s)
    compiler-options-order:   OK (5.16s)
  cabal-to-dhall
    conditional-dependencies: OK
    gh-36:                    OK (0.01s)
    simple:                   OK
    SPDX:                     OK

All 12 tests passed (42.15s)

So, in a bizarre coincidence, we're back to roughly where we were, performance-wise, though some of the individual test numbers have moved and we're running more tests now (though I wouldn't be surprised if that was more due to the parser speed-ups than the typechecking ones). I was half hoping that fixing the big performance loss would've improved things over-all in a big way, but I guess we're not so lucky.

@ocharles ocharles merged commit 05ab27d into dhall-lang:master Jun 3, 2018
@ocharles
Copy link
Member

ocharles commented Jun 3, 2018

Merged! 🎉 Thanks so much for this contribution, it looks like a lot of work went into this.

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.

None yet

3 participants