Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Check PVP import compliance with 'cabal check' #1703

Open
amigalemming opened this Issue Feb 25, 2014 · 16 comments

Comments

Projects
None yet
8 participants

In the libraries@haskell.org mailing list from time to time the issue of package versioning policy compliance arises. There is a recent round starting with Data.Bits.zero. My observation is that programmers try to comply to the PVP on the export side, but not at the import side. That is, if you import identifiers unqualified and implicitly, you also must specify tight version bounds. But most packages don't do that. I like to add the following check to cabal check:

If a package is imported like
Build-Depnds: containers >=0.5 && <0.6
then only qualified imports and explicit imports are allowed, e.g.
import qualified Data.Map as Map
import Data.Map (Map)

In contrast, if the module YourModule imports like
import Data.Map as Map
or
import Data.Map hiding (insert)
then a warning like the following one should be emitted:

"Package imports do not comply with the Package Versioning Policy PVP.
YourModule imports Data.Map inconsistently to the package import of 'containers'.
According to your currently configured package versions you may resolve the problem in three ways:

  1. import qualified Data.Map as XYZ
  2. import Data.Map (x, y, z)
  3. restrict version bounds: containers >=0.5 && <0.5.1"

I think that version ranges like >=0.5 && <1.0 must always be warned about,
and ranges like >=0.5 && <0.5.0.1 are always safe.

Now I am thinking about an implementation. I need to parse the module import parts, also in the presence of comments, TemplateHaskell and C preprocessor directives. Is this already available? I guess I must not add a dependency on the GHC parser or haskell-src-exts to Cabal. I also need access to currently configured package versions, but this one should exist already.

Then I wonder whether this shall be part of Cabal or cabal-install. So far, cabal check is only available in cabal-install, not in Cabal. I don't know the reason for it, but in order to be least invasive I would start extending the 'check' command of cabal-install.

Member

23Skidoo commented Feb 25, 2014

Try implementing this as a separate tool first. Then we can think about adding this functionality to Cabal.

I prepared a first version of such a tool:
https://hackage.haskell.org/package/check-pvp

Owner

tibbe commented Feb 28, 2014

@amigalemming I took it out for a spin. Here's some early user feedback, I hope you don't mind:

  • I have several sections (e.g. library, test suites, and benchmarks) in my Cabal file and I cannot attribute the errors to the sections as the warning don't include line numbers or section names.

  • There are a few too many newlines so the output takes up more than one screen. How about:

    my-file.cabal:6: base: upper bound 5 is too lax
    
  • The tool failed on files that contain CPP. Perhaps because of haskell-src-ext not checking for the CPP pragma? If it's to hard to get haskell-src-exts to do that, perhaps you could look for the extensions directive in the Cabal file instead?

Am 28.02.2014 21:14, schrieb Johan Tibell:

@amigalemming https://github.com/amigalemming I took it out for a
spin. Here's some early user feedback, I hope you don't mind:

  • I have several sections (e.g. library, test suites, and benchmarks)
    in my Cabal file and I cannot attribute the errors to the sections
    as the warning don't include line numbers or section names.

How can I find out line numbers? Cabal gives me a PackageDescription
without line numbers. :-(

Owner

tibbe commented Feb 28, 2014

How can I find out line numbers? Cabal gives me a PackageDescription
without line numbers. :-(

It might be difficult. How about using the section name instead?

library: base: upper bound 5 is too lax
some-test-suite: base: upper bound 5 is too lax

Am 28.02.2014 22:26, schrieb Johan Tibell:

How can I find out line numbers? Cabal gives me a PackageDescription
without line numbers. :-(

It might be difficult. How about using the section name instead?

library: base: upper bound 5 is too lax
some-test-suite: base: upper bound 5 is too lax

I am using flattenPackageDescription in order to get rid of the
conditional branches. But after this transformation I find all
dependencies in PackageDescription.buildDepends and I cannot track back
the origins of the dependencies. :-( Maybe I must choose a different
route through the PackageDescription - any idea?

ibotty commented Mar 1, 2014

some other feedback re check-pvp (which is a great idea).

i get the following error constantly because i have a recent (not yet released) cabal which i install sandboxed.

check-pvp: user error (You need to re-run the 'configure' command. The version of Cabal being used has changed (was Cabal-1.9999, now Cabal-1.16.0).)

i have not looked at the source so i don't know whether this is harmful or not (i guess not, because it works as advertised for my small tests). if so, it would be great to suppress that warning.

Am 01.03.2014 16:55, schrieb Tobias Florek:

|check-pvp: user error (You need to re-run the 'configure' command. The version of Cabal being used has changed (was Cabal-1.9999, now Cabal-1.16.0).)
|

i have not looked at the source so i don't know whether this is harmful
or not (i guess not, because it works as advertised for my small tests).
if so, it would be great to suppress that warning.

I have seen this error myself, but don't know about its origin. At
least, I think that it is important that check-pvp and cabal-install are
linked with the same version of Cabal.

I've seen this error too and it was related to trying to use some cabal operation on a sources of some cabal package after installing a new version of cabal while the package was already cabal-configured.
Steps are like this:

  1. Install cabal-install package version 1.18.0.2
  2. Get sources for some cabal package Foo
  3. Configure package Foo: "cd Foo && cabal configure"
  4. Install some newer version of cabal-install (e.g. 1.19 from cabal source repository)
  5. Try building package Foo.

You will get the above mentioned error (though the versions will be 1.18.0.2 and 1.19)

Running "cabal configure" for package Foo again fixed this issue in my case.

My understanding is that new Cabal tried to use cached LocalBuildInfo for Foo package (stored in dist/setup-config or something like that) and format (and Cabal version reference) has changed thus the error.

Hope this helps

01 ìàðòà 2014 ã., â 20:00, amigalemming notifications@github.com íàïèñàë(à):

Am 01.03.2014 16:55, schrieb Tobias Florek:

|check-pvp: user error (You need to re-run the 'configure' command. The version of Cabal being used has changed (was Cabal-1.9999, now Cabal-1.16.0).)
|

i have not looked at the source so i don't know whether this is harmful
or not (i guess not, because it works as advertised for my small tests).
if so, it would be great to suppress that warning.

I have seen this error myself, but don't know about its origin. At
least, I think that it is important that check-pvp and cabal-install are
linked with the same version of Cabal.

Reply to this email directly or view it on GitHub.

ibotty commented Mar 2, 2014

thanks for your comments. i know why this happens (i am running cabal head and compiled check-pvp with an older cabal library). i was wondering whether this can be mitigated without re-configuring, but it does not seem serious, so i can live with it.

Am 02.03.2014 11:31, schrieb Tobias Florek:

thanks for your comments. i /know/ why this happens (i am running cabal
head and compiled check-pvp with an older cabal library). i was
wondering whether this can be mitigated without re-configuring, but it
does not seem serious, so i can live with it.

If you run check-pvp with the --include-all option, the package
configuration will not be read and thus the Cabal version should not matter.

Owner

hvr commented Apr 26, 2014

Has there been any progress on this one since the last comment?

Am 26.04.2014 13:16, schrieb Herbert Valerio Riedel:

Has there been any progress on this one since the last comment?

Not really. The checks are simple and implemented, however the
integration with Cabal and all preprocessors is not simple. My package
implements two variants: The simple one parses the package description
using readPackageDescription and then reads the modules' contents using
Language.Haskell.Exts.Parser. The advanced one uses haskell-packages and
implements a compiler that can be run by cabal-install. It preprocesses
files and checks also dependend packages but it has no access to the
local build info of the package. I am uncertain about how to proceed.

Owner

hvr commented Apr 26, 2014

@amigalemming would it make sense to add a subset of your checks for starters which do not depend on parsing the files at all for starters? I.e. just perform the sanity checks that make sense even without parsing the import statements (e.g. total omission of bounds, bounds that seem suspicious, like e.g. <= 1.0 or bounds that are overly lax with respect to the currently existing package version in the package index (and maybe suggesting values for the missing/suspicious package bounds like cabal init does)?)

Am 26.04.2014 17:17, schrieb Herbert Valerio Riedel:

@amigalemming https://github.com/amigalemming would it make sense to
add a subset of your checks for starters which do not depend on parsing
the files at all for starters? I.e. just perform the sanity checks that
make sense even without parsing the |import| statements (e.g. total
omission of bounds, bounds that seem suspicious, like e.g. |<= 1.0| or
bounds that are overly lax with respect to the currently existing
package version in the package index (and maybe suggesting values for
the missing/suspicious package bounds like |cabal init| does)?)

I could add an "--exclude-all" option analogously to the "--include-all"
option. It would neither touch the modules nor the package database.

@ttuegel ttuegel added this to the _|_ milestone Apr 23, 2015

This was referenced Apr 24, 2015

Collaborator

BardurArantsson commented Jun 25, 2015

If anyone ever takes a stab at this, see also #465 (point being that this should probably be opt-in via a field in the .cabal file.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment