Got it, thanks.

Mikhail: thank you for the feedback too. If you want me to review any
updates, let me know.

On Tue, Feb 20, 2018 at 10:19 PM, Oleg Grenrus <oleg.gren...@iki.fi> wrote:

> Good point. There aren't ReadP-parser for SPDX.License, so no Text
> instance either.
> But there is `Distribution.Pretty.Pretty` instance (`prettyShow`).
>
> - Oleg
>
>
> On 20.02.2018 20:28, Michael Snoyman wrote:
> > Alright, I've updated the PR to use `Either SPDX.License License`:
> >
> > https://github.com/commercialhaskell/stack/pull/3878/commits/
> 6679aafe99a087dd6aac341fce965f4067e1be77
> >
> > The only difference from your description that I ran into is that
> > there's no Text instance for SPDX.License, meaning instead of:
> >
> >     either display display
> >
> > I ended up with
> >
> >     display . either licenseFromSPDX id
> >
> > Is it intentional that there is no Text instance for SPDX.License?
> >
> > On Tue, Feb 20, 2018 at 8:02 PM, Oleg Grenrus <oleg.gren...@iki.fi
> > <mailto:oleg.gren...@iki.fi>> wrote:
> >
> >     1. The InstalledPackageInfo license field is also `Either
> SPDX.License
> >     License` [1] and you can get a list of IPIs conviently with
> >     `HcPkg.dump` [2]
> >     2. It's up to you, if `Text` is enough, go for it. In the future you
> >     might want to provide some license reports, where you'll need
> >     structured
> >     license information, but then "reverting" `TextualLicense` will be a
> >     type-directed, so it's up to you.
> >
> >     Oleg
> >
> >
> >     -
> >     https://github.com/haskell/cabal/blob/32fea06a1023a507d7dc16b29f5425
> 38c0b55e46/Cabal/Distribution/Types/InstalledPackageInfo.hs#L50
> >     <https://github.com/haskell/cabal/blob/
> 32fea06a1023a507d7dc16b29f542538c0b55e46/Cabal/Distribution/
> Types/InstalledPackageInfo.hs#L50>
> >     -
> >     https://github.com/haskell/cabal/blob/32fea06a1023a507d7dc16b29f5425
> 38c0b55e46/Cabal/Distribution/Simple/Program/HcPkg.hs#L246
> >     <https://github.com/haskell/cabal/blob/
> 32fea06a1023a507d7dc16b29f542538c0b55e46/Cabal/Distribution/
> Simple/Program/HcPkg.hs#L246>
> >
> >
> >     On 20.02.2018 19:45, Michael Snoyman wrote:
> >     > I'm sorry, I'm not quite sure I understand your recommendation. Are
> >     > you saying that I should ideally replace all usages of `License` in
> >     > the Stack codebase with `Either SPDX.License License`? That
> _should_
> >     > be possible, the only questions I'd have are:
> >     >
> >     > 1. We additionally grab license info from the GHC package dump.
> >     Would
> >     > there be any problem parsing that license field into the old
> License
> >     > data type and storing as Right?
> >     > 2. If we're going to have to treat this as arbitrary text anyway,
> is
> >     > there any reason not to represent it as `newtype TextualLicense =
> >     > TextualLicense Text` or similar, and convert immediately with
> >     `display`?
> >     >
> >     > On Tue, Feb 20, 2018 at 6:12 PM, Oleg Grenrus
> >     <oleg.gren...@iki.fi <mailto:oleg.gren...@iki.fi>
> >     > <mailto:oleg.gren...@iki.fi <mailto:oleg.gren...@iki.fi>>> wrote:
> >     >
> >     >     Hi Michael,
> >     >
> >     >     thanks for your comments!
> >     >
> >     >     - The allBuildInfo change is
> >     >
> >      https://github.com/haskell/cabal/commit/
> 8fc10320a5dc4898927c84ad6a2dce7965ef30db
> >     <https://github.com/haskell/cabal/commit/
> 8fc10320a5dc4898927c84ad6a2dce7965ef30db>
> >     >
> >      <https://github.com/haskell/cabal/commit/
> 8fc10320a5dc4898927c84ad6a2dce7965ef30db
> >     <https://github.com/haskell/cabal/commit/
> 8fc10320a5dc4898927c84ad6a2dce7965ef30db>>,
> >     >     I agree with Herbert on this. New `allBuildInfo`
> >     implementation is
> >     >     correct given the name. There was even a TODO to make that
> >     change.
> >     >     `reallyAllBuildInfo` would been silly. I also didn't felt ok
> >     to change
> >     >     the type to `Traversal` (we have lenses, please try out them
> >     too and
> >     >     tell if something is missing!). We'll highlight the change
> >     in the
> >     >     changelog, as it's easy to miss.
> >     >
> >     >     - The lack of SPDX changelog entry is my bad. It was a series
> of
> >     >     patches, and the changelog + manual update is still not
> >     done. I try to
> >     >     summarise:
> >     >         - `cabal-version: 2.2` files use SPDX expression syntax for
> >     >     `license: ` field. PackageDescription's licenseRaw will be
> >     Left expr,
> >     >     expr :: Distribution.SPDX.License.License
> >     >         - `cabal-version: 2.0` files still use old `License`
> >     syntax and
> >     >     type, licenseRaw is `Right l`, l ::
> Distribution.License.License
> >     >         - I recommend treating the field as opaque. You can `either
> >     >     display
> >     >     display` to show it
> >     >            - Next best choice is to use `SPDX.License` as in that
> >     >     direction
> >     >     conversion is lossless (licenseToSPDX), but have to
> >     workaround lack of
> >     >     e.g. `OtherLicense` as a concept (IIRC it's converted to
> >     >     LicenseRef:OtherLicense which is valid SPDX-License-Id).
> >     This might be
> >     >     necessary if you plan to read (human readable) text back.
> >     >
> >     >     Oleg
> >     >
> >     >     On 20.02.2018 15:47, Michael Snoyman wrote:
> >     >     > Hi all,
> >     >     >
> >     >     > Oleg mentioned to me on Reddit that a 2.2 branch has been
> >     cut for
> >     >     > Cabal, and recommended we try to upgrade Stack to it. I'm
> >     sharing
> >     >     > information here on that process in case it's useful to
> >     others,
> >     >     either
> >     >     > as feedback on the API changes, or help for others going
> >     through
> >     >     > similar upgrades. If anyone would rather that I do or do not
> >     >     post such
> >     >     > reports in the future, let me know.
> >     >     >
> >     >     > I've opened a PR for this change[1], which currently is a
> >     single
> >     >     > commit[2]. As you can see from the change to
> >     stack.yaml[3], I needed
> >     >     > to refer to the commit of Cabal in question, and turn on
> >     >     `allow-newer`
> >     >     > for some packages (cabal-doctest and http-api-data) with
> >     restrictive
> >     >     > upper bounds on Cabal. Both of these packages compiled
> without
> >     >     issue.
> >     >     >
> >     >     > No change was necessary to Stack's Setup.hs file.
> >     >     >
> >     >     > Most of the changes so far were compile-time errors, which
> >     is a Good
> >     >     > Thing. Changes I had to make:
> >     >     >
> >     >     > * The build type is no longer a Maybe field. (As a user:
> >     this is a
> >     >     > nice change, no longer a need to guess about what a Nothing
> >     >     value means.)
> >     >     > * There are now two License types, the original one and
> >     the SPDX
> >     >     > variant. I don't understand what this change is about,
> >     some further
> >     >     > explanation in the docs or the ChangeLog would definitely be
> >     >     > appreciated. But hacking my way through the types seems to
> >     have
> >     >     worked.
> >     >     > * Parsing a package description now works on a ByteString
> >     >     instead of a
> >     >     > String. This allowed me to remove some UTF8 conversion and
> >     >     > BOM-stripping code, which is very nice.
> >     >     > * The new parsing API exposes the cabal file version when
> that
> >     >     > prevented the parse (at least, that's my understanding).
> >     The changes
> >     >     > to accommodate the new API were relatively trivial, so
> another
> >     >     +1 here.
> >     >     > * Also: getting file position information for warnings and
> >     errors is
> >     >     > _very_ nice. +1
> >     >     >
> >     >     > I haven't done extensive testing yet, but I've so far only
> >     found one
> >     >     > bug due to runtime changes: the behavior of allBuildInfo
> >     has changed
> >     >     > to now include non-buildable components. This resulted in
> some
> >     >     > confusion until I reviewed the changelog more closely. If I
> >     >     could make
> >     >     > a request, it would be that, in the future, new runtime
> >     behavior
> >     >     come
> >     >     > with a new function name, to convert runtime errors into
> >     compile
> >     >     time
> >     >     > errors. This was _not_ a particularly difficult issue to
> >     work around
> >     >     > though, in large part thanks to the changelog.
> >     >     >
> >     >     > I'll continue testing this branch of Stack. Our plans are
> >     to merge
> >     >     > this to master, and release Stack 1.7.0 close to the Cabal
> 2.2
> >     >     release
> >     >     > date.
> >     >     >
> >     >     > Michael
> >     >     >
> >     >     > [1]
> >     https://github.com/commercialhaskell/stack/pull/3878/files
> >     <https://github.com/commercialhaskell/stack/pull/3878/files>
> >     >     <https://github.com/commercialhaskell/stack/pull/3878/files
> >     <https://github.com/commercialhaskell/stack/pull/3878/files>>
> >     >     > [2]
> >     >     >
> >     >
> >      https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5
> >     <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5>
> >     >
> >      <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5
> >     <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5>>
> >     >     > [3]
> >     >     >
> >     >
> >      https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5#diff-
> fafd0cdcd559a7b124cc61c29413fb54
> >     <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5#diff-
> fafd0cdcd559a7b124cc61c29413fb54>
> >     >
> >      <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5#diff-
> fafd0cdcd559a7b124cc61c29413fb54
> >     <https://github.com/commercialhaskell/stack/pull/3878/commits/
> a101341d04213d6dd8e0cf16d2f2fef8e7ed5cd5#diff-
> fafd0cdcd559a7b124cc61c29413fb54>>
> >     >     >
> >     >     >
> >     >     > _______________________________________________
> >     >     > cabal-devel mailing list
> >     >     > cabal-devel@haskell.org <mailto:cabal-devel@haskell.org>
> >     <mailto:cabal-devel@haskell.org <mailto:cabal-devel@haskell.org>>
> >     >     >
> >     http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel
> >     <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel>
> >     >     <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel
> >     <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel>>
> >     >
> >     >
> >     >
> >     >     _______________________________________________
> >     >     cabal-devel mailing list
> >     >     cabal-devel@haskell.org <mailto:cabal-devel@haskell.org>
> >     <mailto:cabal-devel@haskell.org <mailto:cabal-devel@haskell.org>>
> >     >     http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel
> >     <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel>
> >     >
> >      <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel
> >     <http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel>>
> >     >
> >     >
> >
> >
> >
>
>
>
_______________________________________________
cabal-devel mailing list
cabal-devel@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/cabal-devel

Reply via email to