Change moreRecentFile test to workaround races

[hvr]

This changes moreRecentFile a b to return true not only when a
is younger than b, but also when a is exactly the same age of b, as
that case is subject to race-conditions, and it's better to err on assuming
it needs to be regenerated.

This is an attempt to provide a pragmatic workaround for #2311

[23Skidoo]

This can have unintended consequences. I remember making a similar fix in the past (#1443), which broke something else, so I had to revert it (440fe65).

[hvr]

@23Skidoo alternatively, I could introduce a variant of moreRecentFile that is more explicitly named, so that it's more obvious at the use-sites that == is handled specially...

By now I've found quite a few other cases that suffer from #2311 where I'm torn whether to edit the .cabal files for several packages to declare them incompatible w/ GHC>=7.8 (which wouldn't be accurate).

PS: The reason I changed moreRecentFile was because the Haddock docs seemed to suggest this function is to be used for a very specific use-case, namely that "the second file is generated using the first":

-- | Compare the modification times of two files to see if the first is newer
-- than the second. The first file must exist but the second need not.
-- The expected use case is when the second file is generated using the first.
-- In this use case, if the result is True then the second file is out of date.
--
moreRecentFile :: FilePath -> FilePath -> IO Bool
moreRecentFile a b = do
  exists <- doesFileExist b
  if not exists
    then return True
    else do tb <- getModificationTime b
            ta <- getModificationTime a
            return (ta >= tb)

[23Skidoo]

Looking at the code, moreRecentFile is only used in Cabal/Distribution/Simple/Configure.hs and Cabal/Distribution/Simple/PreProcess.hs. The change is probably OK.

[tibbe]

Please rename the function and document its behavior though.

On Wed, Jan 14, 2015 at 5:00 PM, Mikhail Glushenkov <
[email protected]> wrote:

Looking at the code, moreRecentFile is only used in
Cabal/Distribution/Simple/Configure.hs and
Cabal/Distribution/Simple/PreProcess.hs. The change is probably OK.

—
Reply to this email directly or view it on GitHub
#2362 (comment).

[23Skidoo]

This still won't completely solve #2311, though. When the tarball is extracted, there're 3 possible cases:

1. timestamp(foo.hs) < timestamp(foo.ly)
2. timestamp(foo.hs) == timestamp(foo.ly)
3. timestamp(foo.hs) > timestamp(foo.ly)

Case 1 works fine already; your patch fixes case 2, but case 3 (which I think can also happen) can't be fixed with this approach.

[hvr]

@23Skidoo yeah... that's unfortunately true ... any suggestions?

(cabal sdist seems to place the files in alphabetic order into the .tar file; so case 3 can happen quite easily)

[23Skidoo]

@dcoutts suggested adding metadata to the tarball that'd tell us which version of the preprocessor was used to generate the .hs file (#130).

[hvr]

@23Skidoo

@dcoutts suggested adding metadata to the tarball that'd tell us which version of the preprocessor was used to generate the .hs file

That doesn't help us with all the already existing releases having outdated pre-processed files in them, does it? Do we need to blacklist all of them for GHC >= 7.8?

[23Skidoo]

We can force-regenerate .hs files when the metadata is missing.

[ezyang]

Given that the version of the preprocessor matters for making a package build, maybe this is an indication that it doesn't make sense to upload packages to Hackage with preprocessor output? I mean, I can understand why this original decision was made, but it just doesn't seem good if it causes this sort of problem. (Also, since new-build can automatically install Haskell preprocessors for you, this seems like even less of a good reason.)