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

Resolve circular dependencies #473

Open
swamp-agr opened this issue Jan 10, 2021 · 2 comments
Open

Resolve circular dependencies #473

swamp-agr opened this issue Jan 10, 2021 · 2 comments

Comments

@swamp-agr
Copy link
Contributor

As it was initially discussed in #469, there is circular dependencies between fay and fay-base:

  • fay-base depends on fay.
  • fay-base contains modules with same names as in base (Prelude, Data.Maybe and so on). In order to prevent ambiguous modules names error, it was not exposed by default.
  • fay-tests executable which is hidden by flag test and disabled by default from fay package depends on fay-base.

Thus, fay-tests component could not be converted easily to enabled test-suite.

During discussion in #469 there were 2 proposals opened in order to mitigate circular dependencies:

  1. Move fay-base into fay package as a (separate) library. (multi-component packages are enabled only since cabal 3.0).
  2. Split tests into compiler-related and base-related and convert them to corresponding test-suites.

I would prefer to keep two separate packages and relocate tests accordingly to their purpose (compiler or base library).

@swamp-agr
Copy link
Contributor Author

  • Quick look shown that 185 out of 194 source-based tests failed without fay-base enabled.
  • Test tool as a suite could be compiled without fay-base.
  • @ezzieyguywuf, motivation around splitting package on two parts described in Reduce boilerplate #134.

Options:

Option Pros Cons
Manual control (almost the same, but losing flag -ftest and dependency on fay-base) few changes required new contract for maintainers (almost the same as right now but a bit worse)
Moving the whole test-suite and test sources to fay-base package few changes required fay tests also will be moved to fay-base. fay will live without tests.
Move the most tests to fay-base, leave a few tests in fay Both compiler and base will be covered average code changes: partial code overlap in test-suites, subject of moving test tool into fay library.
Investigate new cabal features (dropping < 8.8 compilers support) and adopt them to move fay-base back inside fay If it is possible then the best of two worlds would be under one package, fay-base could be dropped It is an area of uncertainty: research required. Might be impossible.

@ezzieyguywuf
Copy link

@swamp-agr thanks for your help in getting #469 resolved, and for opening this new issue.

I'm still struggling to get tests working, though, and I think it may be related to something in this bug report, or maybe #459 . Here is how you can reproduce my issue:

# Get the sources
cabal unpack fay
cd fay-0.24.2.0
ls # uh-oh, notice there's no fay-base here...

# Configure and build
ghc Setup.hs -o setup
./setup configure --flag=test
./setup build

# Try to run the tests
export LD_LIBRARY_PATH="${PWD}/dist/build"
./dist/build/fay-tests/fay-tests --num-threads/${nproc} -random 20

This results in many tests failing with ghc-pkg: cannot find package fay-base

If you think about it, this makes sense, as the release tarbal doesn't include fay-base, and the cabal files don't list it at all (you took it out, to resolve the circular dependency: thanks!)

Further investigation shows me how you worked around this on travis, i.e. you just manually install fay-base. However, I can't do this in gentoo, because it drops me right back into the circular dependency issue.

I've seen some packages (for example, parser-combinators) split out tests into a separate package altogether (i.e. parser-combinators-tests). I don't know their motivation for doing so, but it seems that this may be "cheap" for you to implement (fay-tests is already a separate executable) and would resolve my circular dependency problem (I think!).

I don't love that option, though, as it separates the test from the code under test. I guess it is essentially what you've listed in row 2 in your table above.

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

No branches or pull requests

2 participants