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

Enable REST property tests #17

Merged
merged 3 commits into from
May 26, 2019
Merged

Conversation

dimitri-xyz
Copy link
Contributor

This makes the REST tests build and pass (mostly).
I am not sure how to best fix the property test errors due to "orderBookBinning". This pull request is to start that discussion or to learn what the fix should be, if that is clear to you.

Dimitri DeFigueiredo added 2 commits May 15, 2019 19:43
These show up on diffs and clutter our view of changes
1. Two tests still fail, but the best fix is unclear. Left a FIX ME.
2. The name of the test suite `tests` was causing cabal to fail when
running (`cabal new-tests tests`) on the same directory as the .cabal file.
3. Minor logging fix
@Lucsanszky
Copy link
Owner

Thanks for submitting this @dimitri-xyz! I can take a look during the weekend.

@dimitri-xyz
Copy link
Contributor Author

Turns out this is an example of why I don't like to use cabal in my projects.

When I first forked this project and tried to build it, the build failed. I used cabal new-build as it seemed that was the tool you were using. It failed because of a trivial little bug in the use of the logging library katip (item 3 in commit 8295ac4). I assumed you had trivially left it out of a commit by mistake. It is now clear that was not the case.

Turns out that the version of katip in the current hackage index is newer than the one you were using last year, the cabal constraint solver used the latest version and "silently" introduced this bug. My simple fix works for building with cabal, but when I tried to build with stack I found a problem as stack has no snapshot with the newer version of katip that satisfies the package rules in the cabal file:

In the dependencies for haskell-bitmex-rest-0.1.0.0:
    containers-0.6.0.1 from stack configuration does not match >=0.5.0.0 && <0.6  (latest matching version is 0.5.11.0)
    network-2.8.0.0 from stack configuration does not match >=2.6.2 && <2.7  (latest matching version is 2.6.3.6)

As I see it, this bug was introduced because cabal did not provide a reproducible build. (I bet it was building perfectly well when you left it, but it no longer worked when I tried.) Cabal builds are not reproducible unless you freeze them. This is an example of that shortcomming. As far as I know, there are 3 ways to solve this:

  1. hack it: use stack allow-newer (this also ignores lower bounds!)
  2. use cabal new-freeze
  3. use stack and stackage to control dependencies

The first option, does not solve the problem, it just buries it for now. Usually, I'd advise against it.

The second option cabal new-freeze solves the problem (i.e. it provides reproducible builds), but provides no guarantee that the set of packages the dependency solver picks will work nicely together. And it tightly freezes the constraints making it more likely we will encounter cabal hell if all packages use this option.

The third option, using stack/Stackage does guarantee reproducible builds (up to haskell packages) and at least tries to ensure all packages in a single release will work nicely together. There is a catch, though. As I understand it, to upload to hackage one needs to place the version limits in the cabal file (to let the cabal solver work), but this prevents stack from picking the package versions in its own stackage builds. In other words, the 2 tools seem to have conflicting requirements about how to specify dependencies.

Again, one can use the "hacked solution" (1), this will allow people building with stack to have a reproducible build based on whichever resolver they choose (e.g. resolver: lts-13.0) and it will still leave the the version numbers in the .cabal file for uploading to hackage. The problem is that now we will have to build against both tools to make sure the package always works.

My suggestion is to only use stack with allow-newer and to simply copy over the stack constraints to the cabal file when we upload a version to hackage. This lets the cabal solver do its job. The cabal new-build may still fail, just as it failed when I tried to build; but we are not making the situation worse as we know there is at least one set of constraints that works. Namely, the constraints stack uses. This gives us reproducible builds through stack and the ability to upload to hackage for a small extra cost.

This is what I am inclined to use. Let me know your thoughts.

This commit enables building the project with stack rather than cabal.
However, the version built by stack may be *different* from the one built
by `cabal new-build` because of reproducibility issues with cabal.
We should probably refresh and freeze the cabal dependencies.

See:
Lucsanszky#17 (comment)
@Lucsanszky
Copy link
Owner

Hey @dimitri-xyz, sorry for the delay on this one. Sure thing, we can go with stack and thanks for adding support for it. At one point, when I can find some time, I'll probably pick this up: #3
I would like to explore a cabal new-* and nix solution to be more exact. Regardless, we can use stack and even after that I'm sure the two could co-exist. Does this sound good? If so, I'll merge this in.

@dimitri-xyz
Copy link
Contributor Author

That's great! I am all for running cabal new-* and nix too.

@Lucsanszky Lucsanszky merged commit 903916a into Lucsanszky:master May 26, 2019
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.

2 participants