Skip to content

remove fork of Ranged-sets-0.3.0 #273

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

remove fork of Ranged-sets-0.3.0 #273

wants to merge 2 commits into from

Conversation

nicuveo
Copy link
Contributor

@nicuveo nicuveo commented Apr 5, 2025 •
edited
Loading

Alex contains a full copy of Ranged-sets-0.3.0. As noted in NOTE.txt, this was done to reduce the number of dependencies in the Haskell Platform. With the Platform now deprecated, the downsides of maintaining a fork outweigh the advantages. Since there were some local changes, i've opened a PR to upstream the changes to Ranged-sets; they have landed in version 0.5.0.

One of the many upsides of not using a fork is that the original library is fully tested, while the tests were removed in the fork, despite some manual changes being introduced.

This PR adds a dependency on Ranged-sets >= 0.5.0, removes the fork, and deletes NOTE.txt.

@nicuveo
Copy link
Contributor Author

nicuveo commented Apr 5, 2025 •
edited
Loading

Ah, Ranged-sets was upgraded to base >= 4.11 which breaks compatibility with older versions of GHC. There are several ways we can proceed:

  • patch Ranged-sets to go back to base >= 4.10, and use a #if to deal with the Semigroup issue
  • drop compatibility with GHC < 8.4
  • keep the fork, but use a conditional to rely on the library in all recent versions
  • give up on this; don't fix what ain't broke.

What do y'all think?

@Ericson2314
Copy link
Collaborator

I don't mind dropping GHC < 8.4

@nicuveo
Copy link
Contributor Author

nicuveo commented Apr 6, 2025

Ah, another pain point: the emulated workflow is gonna require some trickery, since using cabal to install dependencies segfaults due to the memory constraint. I am not familiar enough with it to figure out how to solve this issue; naively, we could download the dependencies from github and copy the sources into the local tree so that the one call to ghc builds everything?

@Bodigrim
Copy link

Bodigrim commented Apr 6, 2025

I am not familiar enough with it to figure out how to solve this issue; naively, we could download the dependencies from github and copy the sources into the local tree so that the one call to ghc builds everything?

Yes. See https://github.com/haskell/text/blob/5e57460711a9a5ab7f8a30f0e11cd850018dae70/.github/workflows/emulated.yml#L35-L38 for example.

@andreasabel
Copy link
Member

Some facts about Ranged-sets:

  1. It is not part of the GHC shipped libraries.
  2. It is not part of Stackage for GHC < 8.6.

Point 1 means that if we start depending on Ranged-sets we break the current property that alex depends only on the GHC shipped libraries.
Point 2 means that alex would be harder to install for legacy GHCs 8.0-8.4 in conjunction with Stack.

My take on it would be the following (essentially don't change a running system):

  • While we do not find any problems with the current setup (vendoring Ranged-sets-0.3), keeps things as they are.
  • Once we find problems, give up on vendoring so that we do not fix bugs locally that are (already) fixed upstream.

We could also revisit this once we drop GHC < 8.6. (But I would not drop GHCs just for the sake of this cleanup.)

@andreasabel andreasabel added the cleanup Cleaning up / modernizing the code base label Apr 6, 2025
@Bodigrim
Copy link

Bodigrim commented Apr 6, 2025

A compromise could be to make Ranged-sets a submodule, so that it can be easily kept in sync with the upstream.

@nicuveo
Copy link
Contributor Author

nicuveo commented Apr 6, 2025

A compromise could be to make Ranged-sets a submodule, so that it can be easily kept in sync with the upstream.

This wouldn't fix the issue with GHC < 8.4 though.

Sorry for wasting your time everyone: i thought this would be a quick win, but i was wrong. Let's shelve this for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cleanup Cleaning up / modernizing the code base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicuveo @Ericson2314 @Bodigrim @andreasabel