system-f / validation

A data-type like Either but with an accumulating Applicative
Other
99 stars 28 forks source link

Updating to use lens-5 #44

Closed recursion-ninja closed 3 years ago

recursion-ninja commented 3 years ago

This is a breaking change to use lens-5 and make validation compatible with ghc-9.0. I bumped the version to validation-1.2 as this would be a breaking change. validation-1.1 would support pre-ghc-.9.0 and validation-1.2 would support ghc-9.0 and potentially other versions moving forward.

recursion-ninja commented 3 years ago

@tonymorris, @gwils any update on merging this PR and uploading a new version of validation to hackage which can be built with ghc-9.0?

tonymorris commented 3 years ago

Can we not support GHC before and including 9.0?

recursion-ninja commented 3 years ago

@tonymorris Sorry for the delay on replying. I attempted to modify the pull request to support GHC-9.0. and* previous GHC versions.

I tested fro support by running the following commands:

$ git checkout 809887152ceaab664c5fc7ed02dcf5566b4a812d
$ for cmd in "9.0.1" "8.10.4" "8.8.4" "8.6.5" "8.4.4" "8.2.2" "8.0.2" "7.10.3" "7.4.2"; do cabal build --with-compiler=ghc-$cmd; done
$ git checkout f253642d96eaf0024bd858bb0f2bd60b0d5079ef
$ for cmd in "9.0.1" "8.10.4" "8.8.4" "8.6.5" "8.4.4" "8.2.2" "8.0.2" "7.10.3" "7.4.2"; do cabal build --with-compiler=ghc-$cmd; done

I successfully built the current commit (8098871) with the following GHC versions:

I did test version 7.4.2, however generic-deriving, a transitive dependency of lens, resulted in a segmentation fault:

cabal: Failed to build generic-deriving-1.14 (which is required by
validation-1.2). The build process segfaulted (i.e. SIGSEGV).
Failed to build text-1.2.4.0 (which is required by validation-1.2). The build
process segfaulted (i.e. SIGSEGV).
Failed to build text-1.2.4.1 (which is required by validation-1.2). The build
process segfaulted (i.e. SIGSEGV).

I would like to note that the current HEAD of master (f253642) results in the same segmentation fault when building with ghc-7.4.2.

Summary

  1. The current state of the PR adds support for ghc-9.0.* and does not limit support for previous GHC versions (which previously built successfully).
  2. We may want to remove GHC==7.4.2 from the tested-with stanza in the .cabal file.
gwils commented 3 years ago

It would be good to get github actions testing set up for this repository. I think working as far back as 7.10 is generous at this point in time, so this sounds good to me. Maybe we should also remove 7.8 and 7.6 from the tested-with clause.

recursion-ninja commented 3 years ago

@gwils I can add GitHub actions to this commit as well if you'd like.

gwils commented 3 years ago

I don't see it as a prerequisite for merging this, but that would be awesome if it's not too much trouble.

recursion-ninja commented 3 years ago

@gwils I added .github/workflows/ci.yml. I believe that one of the repository maintainers will need to enable GitHub Actions for this to run. There should be a "New Workflow" button under the Actions tab. Please ping me when the GitHub Actions are enabled, and I can make sure that this CI plan works correctly for the validation package.

Note that I added a lower-bounds.cabal file which sets the constraints to match the lower bounds of each dependency listed in the validation.cabal file. I had to bump up the lower bounds a bit to successfully compile with the specified lower bounds and ghc-7.10.3. However, now there is a CI check included in the GitHub Actions which uses lower-bounds.cabal to ensure that the validation package builds with the lowest specified version of each dependency and the lowest GHC version with is listed as tested-with.

gwils commented 3 years ago

@tonymorris will need to do that, or give me permissions on the repo to do it

gwils commented 3 years ago

@recursion-ninja I think there's no "enable" step with github actions - we just need the .yml file in master as far as I can tell. Can you please make a separate PR with just the ci.yml changes, and I'll merge that one first.

recursion-ninja commented 3 years ago

@gwils Huh, well it has been a while since I set up a workflow via GitHub Actions. I'll create a new branch off of master and a new pull request for setting up the CI via GitHub actions. I'll also rollback the changes on this branch to only reflect the lens-5 and ghc-9.0.* compatibility changes.

recursion-ninja commented 3 years ago

@tonymorris @gwils See #47. I'm going to wait until that pull request is merged before rolling back commits for the lens-5 and ghc-9.0.* support.

gwils commented 3 years ago

Looks like Tony has merged #48 instead of this one. Thank you for your effort. I'd like to get your CI changes through on the other branch.