Broken on GHC 7.4

[ndmitchell]

1.20.4 is broken on GHC 7.4.2, see the log at https://travis-ci.org/ndmitchell/hlint/jobs/207295898

My reading of this is that the 1.20.4 version has an insufficient lower bound on directory, as findFile was only introduced later on (unfortunately their changelog doesn't go back far enough to know exactly when).

[RyanGlScott]

cc'ing @jmitchell, who suggested this change in #8.

[jmitchell]

@malcolmwallace, I believe cpphs needs to to have directory >= 1.3.1.0 set in the cabal file, both for the executable and library.

The fix for #8 initially didn't work as expected due to a misunderstanding of directory's findFile behavior on Windows. After that, findFile's specification and implementation were improved. Now the patch introduced in #8 should work as expected with directory >= 1.3.1.0.

[RyanGlScott]

Be aware that GHC 7.4 and earlier are shipped with versions of directory that are older than 1.3.1.0 (and thus don't include findFile). While you can install a more recent version of directory, you cannot use a newer version of directory than the one GHC shipped with and simultanouelsy depend on the ghc library (which would preclude things like using cpphs in a GHC 7.4 plugin, for instance).

Not saying that this is a dealbreaker, but it's something worth keeping in mind.

[jmitchell]

Ah, thanks for pointing that out, @RyanGlScott.

Perhaps the safest way forward is to implement a different fix for #8 that doesn't rely on findFile.

[jmitchell]

This patch may do the trick. It has not yet been extensively tested on a variety of operating systems and GHC versions.

Review and help testing welcome :)

hunk ./Language/Preprocessor/Cpphs/ReadFirst.hs 21
-import System.Directory (doesFileExist,findFile)
-import System.FilePath  (isRelative)
+import System.Directory (doesFileExist)
+import System.FilePath  (isRelative, (</>))
hunk ./Language/Preprocessor/Cpphs/ReadFirst.hs 59
+    findFile [] fname = do
+      exists <- doesFileExist fname
+      if (not $ isRelative fname) && exists
+        then do return $ Just fname
+        else do return Nothing
+    findFile (d:ds) fname = do
+      let path = d </> fname
+      exists <- doesFileExist path
+      if exists
+        then do return $ Just path
+        else findFile ds fname
+
+
hunk ./Language/Preprocessor/Cpphs/ReadFirst.hs 87
-

[jmitchell]

To ease cpphs testing I've started a new project: https://github.com/jmitchell/cpphs-blackbox

It's still very much a skeleton based on @ndmitchell's neil utility (technically my fork) and Travis/AppVeyor CI. The plan for the first iteration is to:

• [x] revert from my fork of neil
• [x] install darcs
• [x] pull the cpphs repo into a 3rd party directory
• [x] add sanity test
• [x] get it passing in Linux/Windows and across multiple GHC versions

The eventual goal is to support test builds against an arbitrary darcs patch submitted via PR. This should help catch problems before they're merged, and give @malcolmwallace more confidence in proposed patches.

For today I'm done working on cpphs-blackbox, but PRs for the above tasks are welcome. Otherwise I'll return to it periodically as I have time.

If this approach ends up working well, perhaps we can migrate the code to this GH project.

[ndmitchell]

Perhaps @malcolmwallace could be persuaded to move the code to github - I know he knows how to use git nowadays. Then the neil script could work unchanged, and it would be easier to accept pull requests and trigger CI builds etc.

[jmitchell]

@ndmitchell, that would be great, and eventually I think that's where the work should end up. @malcolmwallace, are you open to moving cpphs to GitHub or using this repo to host the blackbox testing and CI code?

Most of the changes in my fork aren't actually necessary, but I think this one is. neil currently assumes the repo under test is hosted under your (@ndmitchell's) username. Should be easy to generalize that, though.

[ndmitchell]

@jmitchell - I'd happily accept a patch to compute the user name.

One word of warning with neil - the set of tests evolves over time. For example at some point I'll change the GHC versions permissible to require GHC 8.0.3 instead of 8.0.2 (if such a release ever gets made). That results in occasional maintenance work. An easy way to work around that is to bake the SHA1 of the git version into the bootstrapping script, and arrange to pass it onwards, so that future changes only impact you if you change the SHA1.

[jmitchell]

@malcolmwallace, can you consider merging this patch?

I manually verified in Windows that it still does the right thing with both absolute and relative paths. This Travis build replicates the issue raised here by Neil, and this build applies that patch and resolves the issue.

[jmitchell]

By the way, @malcolmwallace, if you're willing to migrate to git to simplify CI this project might help: darcs-to-git.

[domenkozar]

Excuse my ignorance, but I couldn't find how long is each GHC major version supported. GHC 7.4 is 5+ years old release, how long does community support that effort?

[ndmitchell]

@domenkozar it varies. A lot of the Stack/Yesod crowd do current and previous release. A lot of the Cabal/Hackage crowd at least ensure that incompatibilities with older versions are documented and tested way back to GHC 7.0. I typically test back as far as I can, e.g. 7.4, but if the 7.4 incompatibility becomes an issue, I am fairly relaxed about dropping it - it's usually very little to maintain compatibility for most packages. As an example, my nightly HLint builds are currently failing due to this release, and I'll probably just stop testing 7.4 if this doesn't get fixed in a week or so.

In this case cpphs requires newer features of directory, but doesn't say it does. That happens to cause an issue on 7.4, but you can quite legitimately argue it's wrong not to include those lower bounds even if the problem is only detected on 7.4.

[jmitchell]

I'd like to keep this issue focused on the proposed patch to fix this and #11 (see https://github.com/malcolmwallace/cpphs/issues/9#issuecomment-285122025). (cc: @malcolmwallace)

I've opened #12 for more focused discussion about CI, hosting on GitHub, migrating to git, GHC version support, etc.

[malcolmwallace]

As in #11, I have rolled back the problematic change in 1.20.4, and deprecated 1.20.4 on Hackage.

[ndmitchell]

@malcolmwallace - I think you can close this bug then?

[andreasabel]

@malcolmwallace Could this issue be closed?

[malcolmwallace]

Closed.