search

typelead / etlas

Etlas, the build tool and package manager for the Eta programming language
63 stars 10 forks source link

Make etlas.dhall the default configuration file for etlas #66

Open jneira opened 5 years ago

jneira commented 5 years ago

As described in https://github.com/typelead/eta/issues/704#issuecomment-411348461. Here we can track the progress and specific issues about. The plan would be:

jneira commented 5 years ago

I manged to implement a initial version that uses etlas.dhall but testing it with https://github.com/jneira/wai-servlet/blob/master/etlas.dhall and i've got an error :disappointed: :

Error:
    Dependency on unbuildable (i.e. 'buildable: False') library from base
    In the stanza 'library'
    In the inplace package 'wai-servlet-0.1.5.1'
rahulmutt commented 5 years ago

@jneira Can you share the output with -v3? It may be failing inside of etlas-cabal.

rahulmutt commented 5 years ago

Also can you add a debug log in etlas that dumps the parsed GenericPackageDescription from the etlas.dhall file?

rahulmutt commented 5 years ago

@jneira Any particular reason for:

https://github.com/jneira/etlas/commit/5048002c26d03dd14dc469361f520f0689f3da7c#diff-2d388570b233aa3bd5298e03db1e93c8L512

Shouldn't it be ["./etlas.dhall", "./*.cabal"]?

jneira commented 5 years ago

That was my first version but then etlas requires both files and throwed an error if etlas.dhall was not present. Maybe it should change to make it required only one of them.

rahulmutt commented 5 years ago

Oh right, that field is for required files. Then it looks ok.

jneira commented 5 years ago
rahulmutt commented 5 years ago

@jneira Maybe you can try populating buildDepends in dhall-to-etlas temporarily to see if that fixes it?

jneira commented 5 years ago

Mmmm, that would be a solution but as i have to see how etlas does it maybe i can see where it does and hopefully fix in etlas itself, if possible

rahulmutt commented 5 years ago

Sure that works too!

jneira commented 5 years ago

The problem seems to be in the initial parse of config files, comparing the GenericPackageDescription generated by cabal internal parser and dhall-to-etlas we can check that dhall-to-etlas is not filling the CondNode.condTreeConstraints in any component. There is the info of conditional dependencies of the component. That difference was not catched by dhall-to-etlas tests cause they are testing the output of showGenericDescription and it is equal without condTreeConstraints info

rahulmutt commented 5 years ago

Does filling in those fields properly fix the issue you were hitting?

jneira commented 5 years ago

@rahulmutt yeah, i think dhall-to-etlas should fill it when parsing the file in this function. In the meanwhile i've change the code to reparse the GenericPackageDescription from dhall with Cabal itself although it is terribly inefficient. Otoh thinking twice about defaultImplicitProjectConfig.defaultPackages i think we cant left it empty and use cause using projectPackagesOptional makes cabal build the package twice, one using dhall fille and another one using the cabal file. As it uses "glob" to find files we cant use (./etlas.dhall)|(./*.cabal) to use one file or another.

rahulmutt commented 5 years ago

@jneira That workaround seems OK for now, let's just focus on correctness first and then we'll worry about performance.

To fix that problem of building package twice, we need to do a filtering before returning with the concat arguments in findProjectPackages. The filtering works like this:

Find all ProjectPackageLocalCabalFile and ProjectPackageLocalDhallFile that share the same parent directory. If such pairs are found, filter out the ProjectPackageLocalCabalFile and go with the ProjectPackageLocalDhallFile.

jneira commented 5 years ago

Hi, i have a initial working version of etlas with dhall integration I've tested most of etlas commands with this repo: https://github.com/jneira/eta-test/tree/test-dhall. The tests had been simple: run the command without args but -v3 with dhall file, cabal one and both, without dependencies and with dhall/aeson. The checklist of dhall integration by command:

Pending:

Jyothsnasrinivas commented 5 years ago

This is great news @jneira! :D

jneira commented 5 years ago

Performance of parsing dhall config files:

So we must use the cached version whenever possible. The computation of the hash takes the same time that parsing itself (cause dhall caches the normalized version) so we should find out the way to get the content and the hash at same time, in one step. Then we should save the import hashed dhall file in dist/cache/import.etlas.dhall and use it while the modification time of the original etlas.dhall file is older. I am afraid that those 17 seconds are unavoidable in each etlas.dhall file change but let's hope the performance of dhall parsing improves.

rahulmutt commented 5 years ago

Great analysis @jneira! That 17 seconds is a bit much. I think a performance target should be to get it down to at least 3 seconds - it's OK if it's a couple seconds slower than the .cabal file format since it adds more power/re-use.

Users will typically not change their .cabal files very often, but they shouldn't have to think twice to make changes b/c of perf issues!

jneira commented 5 years ago

Well, another option is to use a derived .cabal file as "cache" file cause it is parsed faster. In that way users will have to wait 10 seconds in each dhall file change.

jneira commented 5 years ago

After updating etlas to use dhall-1.18.0 and dhall-to-etlas-1.4.0.0 the performance has improved but continue being not satisfactory:

jneira commented 5 years ago

Hi, i've done some tests with the new config files, <pkg-name>.etlas and etlas.dhall:

The unique test that fails is etlas old-install eta-test-0.2.0.0 (the version with etlas.dhall):

etlas.exe: Error Parsing: file "dist\etlas.dhall.cabal" doesn't exist. Cannot continue. etlas: Leaving directory 'C:\TEMP\etlas-tmp-8040'



So the dhall file is parsed and used, the derived cabal file writed (not error in that step) but `eta` does not find it. Maybe the write is failing silently cause the dist folder soes not exist... to investigate and fix :thinking:
rahulmutt commented 5 years ago

From a cursory glance at the logs, I notice that the dhall parsing code uses relative paths, so perhaps the derived cabal file is being written to the dist directory of the current directory of the etlas process? Perhaps you need to be writing to C:\TEMP\etlas-tmp-8040\dist\etlas.dhall.cabal?

etlas build works with a dist folder relative to the current directory, so it makes sense that it works. But there is a case in which it doesn't:

directory structure

- eta-test
  - eta-test2
    - etlas.dhall (some basic stub file)
  - cabal.project

cabal.project

packages: eta-test2

Run the following:

cd eta-test2
etlas build

In this case, it should create the dist directory inside of eta-test and not inside of eta-test2 since new-build system uses a centralized dist directory for all the packages you're building for a given cabal.project file. Hence, if relative directory mismatch is the problem, probably this should fail as well? Unless the new-build codepath for dhall passes in the paths properly.

jneira commented 5 years ago

The performance of near version of dhall-to-etlas 1.4.0.0 starts to be reasonable:

golden tests
  dhall-to-cabal
    wai-servlet:               OK (3.48s)
    wai-servlet-handler-jetty: OK (3.34s)
    nested-conditions:         OK (3.07s)
    mixins-no-signatures:      OK (2.97s)
    map-source-repo:           OK (2.99s)
    gh-55:                     OK (3.23s)
    gh-53:                     OK (3.13s)
    empty-package:             OK (0.59s)
    dhall-to-etlas:            OK (4.22s)
    default-license-2.2:       OK (2.84s)
    conditional-dependencies:  OK (3.65s)
    compiler-options-order:    OK (3.48s)
    allrightsreserved-2.2:     OK (2.92s)
    allrightsreserved-2.0:     OK (2.84s)

:laughing:

rahulmutt commented 5 years ago

That's amazing @jneira! Thank for you all the work for making dhall practical :)

jneira commented 5 years ago

With #83 we'll got the new performance improvements and the fix to etlas old-install of remote packages with etlas.dhall files.

jneira commented 5 years ago

The performance of etlas using dhall has improved and it is below 5 seconds:

rahulmutt commented 5 years ago

However we need to cache it in each change and it would be still slow. Possible solutions:

* Use a derived cabal file as "cache" cause it is fast to write and read

This sounds like a great idea.

* Launch the dhall caching in parallel in the first build after change, to not slow down.

Can you elaborate on this? So are you saying that it's taking 4.75 seconds because caching is happening in addition to parsing? w/o caching how much time would it take?

jneira commented 5 years ago

The second option would be read the changed dhall file (in 4.75 s), start to build immediately in one thread and start another one to cache the readed data (that hopefully will end in 4 seconds aprox.) Not sure how can interact both processes and maybe it is too much work to avoid 4 seconds.

jneira commented 5 years ago

Finally we've implemented a cache of etlas.dhall using builtin dhall cache mechanism: #88 But the cache invalidation depends on the hash of dhall.etlas, each change in the file will trigger the read and cache steps.

jneira commented 5 years ago

@rahulmutt has made good points about the usage of the dhall format in etlas config context: see https://github.com/typelead/eta/issues/946

jneira commented 5 years ago

With #96 etlas init will generate an etlas.dhall by default.

jneira commented 5 years ago

possible improvements

jneira commented 5 years ago

To make etlas.dhall more resilient and make it work oflline we should (as suggested in https://github.com/dhall-lang/dhall-lang/issues/378):

rahulmutt commented 5 years ago

@jneira Sounds like a nice idea. Do you think this would make the performance even better?

jneira commented 5 years ago

Definitely it will for dependencies.dhall cause it can change and we should not freeze with a hash by default. I was thinking in downloading it as part of etlas update.

rahulmutt commented 5 years ago

Perhaps the easiest way is to just add it to etlas-index? No need to add additional logic other than to cache it.

jneira commented 5 years ago

I've requested a pr to bump up dhall version to 1.23: #102 This version has improved nomalization perf so:

Reading and caching package configuration from dhall file: .\etlas.dhall
Configuration readed in 4.836031 seconds
rahulmutt commented 5 years ago

@jneira That's wonderful! What are the remaining tasks for this issue?

jneira commented 5 years ago

Mmm, i think only left use a local copy of imports as suggested in https://github.com/typelead/etlas/issues/66#issuecomment-465457431

jneira commented 4 years ago

I've updated dhall support for 1.26.1 of dhall-to-etlas and etlas: https://github.com/typelead/etlas/pull/108