Check PVP import compliance with 'cabal check'

[amigalemming]

In the [email protected] 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.

[23Skidoo]

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

[amigalemming]

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

[tibbe]

@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?

[amigalemming]

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. :-(

[tibbe]

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

[amigalemming]

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]

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.

[amigalemming]

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.

[maximkulkin]

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 [email protected] íàïèñàë(à):

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]

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.

[amigalemming]

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.

[hvr]

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

[amigalemming]

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.

[hvr]

@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)?)

[amigalemming]

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.

[BardurArantsson]

If anyone ever takes a stab at this, see also https://github.com/haskell/cabal/issues/465 (point being that this should probably be opt-in via a field in the .cabal file.)