Skip to content

Add support for cabal-2.0 caret operator & trailing-zero normalisation #2

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

Closed
hvr opened this issue Apr 8, 2017 · 13 comments
Closed

Comments

@hvr
Copy link

hvr commented Apr 8, 2017

This tool looks very promising, I'm glad you've written it!

I was looking into adding support myself, but noticed that staversion relies upon Cabal for version range handling. So I guess this needs to wait till Cabal-2.0 is officially released. Also I'm not sure how you want the CLI to be designed. I think there's two optional modifiers which I'd like to have available in the CLI:

  1. normalise trailing zeroes (allows to simplify version ranges even more)
  2. enable use of ^>= operator (requires cabal-version:>=2.0)

trailing-0 normalisation

A range such as

    base >=4.8.2.0 && <4.9 || >=4.9.0.0 && <4.10,

can be simplified even more, if the invariant can be assumed that there can only be base-4.9 or base-4.9.0.0 uploaded to Hackage, but not both:

    base >=4.8.2 && <4.9 || >=4.9 && <4.10,

which consequently simplifies into

    base >=4.8.2 && <4.10,

(See also haskell/hackage-server@27f9727).


^>= operator

With the new ^>= operator (documented here), the example

    $ staversion --aggregate pvp -r lts-5 -r lts-6 -r lts-7 -H aeson
    ------ lts-5 (lts-5.18), lts-6 (lts-6.31), lts-7 (lts-7.20), latest in hackage
    aeson >=0.9.0.1 && <0.10 || >=0.11.3.0 && <0.12 || >=1.1.1.0 && <1.2

can be written more tersely as (--caret is just a strawman-proposal)

    $ staversion --aggregate pvp --caret -r lts-5 -r lts-6 -r lts-7 -H aeson
    ------ lts-5 (lts-5.18), lts-6 (lts-6.31), lts-7 (lts-7.20), latest in hackage
    aeson ^>=0.9.0.1 || ^>=0.11.3.0 || ^>=1.1.1.0

and this can be combined with the zero-normalisation via e.g. --zero-norm (again, strawman-proposal, open for bikeshed):

    $ staversion --aggregate pvp --caret --zero-norm -r lts-5 -r lts-6 -r lts-7 -H aeson
    ------ lts-5 (lts-5.18), lts-6 (lts-6.31), lts-7 (lts-7.20), latest in hackage
    aeson ^>=0.9.0.1 || ^>=0.11.3 || ^>=1.1.1

Which IMO is more concise and more readable than the original version-range-union staversion currently emits. Besides readability, another reason I introduced the new caret operator in Cabal is that formally two different major versions of a package can be effectively two totally different APIs (in the extreme case; there's a few examples on Hackage for that), and the use of || visually hints at this property IMO a bit better than having a merged version range >= 0.9 && < 1.2 which IMO gives the wrong impression.


What do you think?

/cc @phadej @bergmark

@debug-ito
Copy link
Owner

Thanks for starting this discussion!

In my optimistic CLI design, your two cases would make independent aggregators. For example,

$ staversion --aggregate pvp-trailing-0-norm

and

$ staversion --aggregate pvp-traling-0-norm-caret

Of course, this doesn't scale at all :)

I think it is a little controversial how we should write build-depends sections. Everyone may have their own policy and preference about it.

So ultimately in my opinion, staversion should take external aggregators via some kind of plugin mechanism, so that users develop their own aggregators and inject them to staversion. If we design the plugin mechanism clever enough, we should be able to distribute aggregator plugins as cabal packages.

However, if there are not so many potential aggregators we should use, I think we can still rely on single --aggregator option.

Below is my personal opinion about the two options you proposed:

trailing-0 normalisation

I didn't know the discussion about the trailing-0 version numbers. In fact, your very example of base was getting on my nerves, but the normalisation totally solves the problem!

So, I think I can live with --aggregate pvp-trailing-0-norm (I need a shorter name, though..)

^>= operator

This may be a bit controversial.

I don't care much about readability of my build-depends. I care more about buildability. So I would stick to using >=X && <Y syntax rather than requiring users to use Cabal-2.X. Of course, requirement for Cabal >=2.0 would eventually be no problem, but it should take some time..

@hvr
Copy link
Author

hvr commented Apr 8, 2017

Fwiw, I don't think there's gonna that many aggregators (but I can think of yet another one: PVP-bounded by minor versions).

So I would stick to using >=X && <Y syntax rather than requiring users to use Cabal-2.X. Of course, requirement for Cabal >=2.0 would eventually be no problem, but it should take some time..

You're referring to the build dependencies of staversion? If so, we can easily compute the ^>=-representation even without needing to compile staversion against Cabal-2.0. It's just a bit more boilerplate...

@debug-ito
Copy link
Owner

You're referring to the build dependencies of staversion?

No. I meant build-depends of a package that one uses staversion to generate. I would not use ^>= syntax in my own packages (at least for a while), so I would want staversion to generate build-depends with >=X && <Y syntax (of course this doesn't mean it shouldn't have an option to generate ^>= syntax, though.)

As for the build dependencies of staversion, I understand your point. We can implement ^>= format without waiting for Cabal-2.0.

This issue can be divided into two components.

  1. Choice of Aggregator, which is a type-synonym for NonEmpty Version -> VersionRange. --aggregate option controls this.
  2. Choice of formatter VersionRange -> String. Currently I just let Cabal do it (see https://github.com/debug-ito/staversion/blob/0.1.4.0/src/Staversion/Internal/Aggregate.hs#L51 )

"trailing-0 normalisation" is related to the component 1, while ^>= operator is related to the component 2, right?

So, my plan is:

  1. List up potentially useful Aggregators, including "PVP with trailing-0 normalisation".

    • If there are not so many of them, we can just implement them as separate --aggregate options.
    • By the way, I think we can just add "trailing-0 normalisation" to "pvp" aggregator. Although it's a backward-incompatible change, it's a minor one.
    • If there are too many, we will need another plan then..
  2. Add a new option (--format-version?) to control VersionRange -> String function to use.

    • and add "caret" format to enable ^>= syntax.

@hvr
Copy link
Author

hvr commented Apr 11, 2017

I totally agree that staversion should default to create cabal-version:>=1.10-compatible build-depends for now, while allowing to use the ^>= operator if the user opts-in explicitly to cabal-version:>=2.0 compatibility (either because the .cabal file already declares cabal-version:>=2.0 or because of CLI flags).

Making the 0-normalisation the default sounds reasonable to me. I think I've already mentioned all aggregators that make sense to me (in the context of Hackage's PVP policy), i.e. specifically

  • PVP w/ major-ver-upper-bounds w/ 0-norm (w/ or w/o caret)
  • PVP w/ minor-ver-upper-bounds w/ 0-norm (caret makes no sense here IMO)

I don't think anyone will need to disable 0-normalisation other than for debugging/testing purposes.

PS: Btw, does staversion allow to inherit existing version bounds in the .cabal file? (in the style of what stack sdist --pvp-bounds=both does)?

@debug-ito
Copy link
Owner

Thanks for your opinion! I'll (re)design --aggregate options first. And then, I'll deal with the caret format.

does staversion allow to inherit existing version bounds in the .cabal file?

No, it doesn't.

staversion uses a home-made parser for .cabal files, which can just extract package names from build-depends sections. I tried to use the parsers from Cabal package, but they had some problems (e.g. they shuffle the package names, they are supposed to be internal etc.) I knew there were some attempts to implement parsers more friendly to external code (haskell/cabal#3614, haskell/cabal#2865.) Probably they can help in future.

Even if there were good parsers, however, I don't think staversion should have the feature of merging version ranges from .cabal files. It's too much for one executable. In my opinion, staversion's role should be just to fetch versions for queried packages. To implement merging, I would refactor staversion's internals into public library modules (see #1.) With those modules and good Cabal parsers, we would be able to write merging logics easily.

@debug-ito
Copy link
Owner

I wrote a test spec for aggPvpMinor (PVP w/ minor-ver-upper-bounds w/ 0-norm)

@hvr if you have time, could you review it? They are just pairs of input versions and the output version range.

@hvr
Copy link
Author

hvr commented Apr 28, 2017

@debug-ito Awesome! I'll try to, but I'm a bit busy right now w/ preparations for GHC 8.2.1RC2 & Cabal 2.0 coming up, so it'll probably be some time till I get to it. Can you remind me sometime next week again?

@debug-ito
Copy link
Owner

OK, I will. Thanks!

@debug-ito
Copy link
Owner

@hvr Hi! Do you have time for review?

On pvp_agg branch, I implemented pvp-minor and pvp-major aggregator options. pvp-major is just an alias for pvp.

@hvr
Copy link
Author

hvr commented May 13, 2017

@debug-ito I just gave it test-drive, and the results look good to me!

@debug-ito
Copy link
Owner

Thanks! I'll release it.

@debug-ito
Copy link
Owner

Just released staversion-0.2.1.0, which includes --format-version cabal-caret option.

@debug-ito
Copy link
Owner

If the option formats versions incorrectly, just open a new issue.

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