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

Quick proof of concept refactoring of `VersionRange` #5098

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@hvr
Copy link
Member

hvr commented Feb 2, 2018

This is a preparatory refactoring for exploring/evaluation how to handle introducing a separate Version type for pkg-config version numbers (which have a richer syntax/grammar, such as openssl-1.0.2j and according to the pkg-config docs uses RPM-style version comparision semantics, see http://blog.jasonantman.com/2014/07/how-yum-and-rpm-compare-versions/ for the details).

The current refactoring is effectively limited to modifications in a single file, by introducing type syns for compatiblity. I've only tried compiling lib:Cabal, which did build fine. I'd expect the heap-representation and the Binary instances to be unaffected by this refactoring.

The ugliest part is for me is the current ad-hoc IsVersion class hack that factors out the Version-specific logic.

The reason we need this, is because of

-- | Describes a dependency on a pkg-config library
--
-- @since 2.0.0.2
data PkgconfigDependency = PkgconfigDependency
                           PkgconfigName
                           VersionRange
                         deriving (Generic, Read, Show, Eq, Typeable, Data)

which currently uses a VersionRange; but if we want to introduce a PkgconfigVersion type, we'd have to

  1. either duplicate VersionRange (into a PkgconfigVersionRange) or
  2. abstract the current one over the "version field".

And this refactoring explores the 2nd option, which tries to reduce code duplication (think also of e.g. how the solver code would be affected).

Any suggestions/ideas?

@hvr hvr requested review from phadej and 23Skidoo Feb 2, 2018

-- But that results in a cycle in the type-syn definition...
--

type VersionRangeF a = VersionRangeOverF Version a

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

i'd omit eta-reduce, will make type-synonym more useful.

class Ord v => IsVersion v where
dispWild :: v -> Disp.Doc
noVersion :: VersionRangeOver v
majorUpperBound :: v -> v

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

need to preserve haddocks.

This comment has been minimized.

Copy link
@hvr

hvr Feb 2, 2018

Author Member

true; I didn't bother yet, as this was just a quick proof of concept; but haddocks definitely need polish if this PR shall proceed.

-- to construct a range @>= 0.4.1 && < 0.5@
--
-- @since 2.2
majorUpperBound = alterVersion $ \numbers -> case numbers of

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

because instance member haddocks are IMHO hidden quite well.

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

also what will ^>= mean for PkgConfigVersion? Should that be disallowed? structurally or in cabal check?

This comment has been minimized.

Copy link
@hvr

hvr Feb 2, 2018

Author Member

Well, ^>= assumes a well established semantic versioning policy/convention in order to justify an inferred first-order approximation compatibility neighbourhood. This doesn't exist for pkg-config(1) versions. The meaning of ^>= for pkgconfig is thus IMO not well-defined (and would be at best rather arbitrary), and we should IMO discourage its use in pkgconfig-depends until we come up with a good one. So at the very least a cabal check warning/error is in order.

noVersion :: VersionRangeOver v
majorUpperBound :: v -> v
wildcardUpperBound :: v -> v
verOrWild :: CabalParsing m => m (Bool, v)

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

this is ugly.

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

... I mean, we probably need an extension point to allow version specific VersionRange syntax constructs. And this isn't the prettiest one. Have to think what would work best (given Version and PkgConfigVersion differences).

IMO openssl ==1.0.1* isn't something we want to allow. I don't know what pkg-configs own syntax looks like.

This comment has been minimized.

Copy link
@phadej

phadej Feb 2, 2018

Collaborator

One idea is to bundle versionRangeReadP as a whole into IsVersion

This comment has been minimized.

Copy link
@hvr

hvr Feb 2, 2018

Author Member

This is indeed the part I'm most unsure/unhappy about.

@idontgetoutmuch

This comment has been minimized.

Copy link
Contributor

idontgetoutmuch commented Mar 2, 2018

FYI - I was installing HEAD of the gnu scientific library (gsl) and it had a version of

4.5+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.