Hi! My comments on the patch.
On Thu, Sep 18, 2008 at 14:33:06 -0400, David Roundy wrote:
> This particular set of patches just cleans up the build to be less
> reliant on autoconf. It also probably breaks the ghc 6.4 build, so
> it's probably better for a post-2.0.3 kind of change (so I'm not
> pushing this just yet).
Yeah. This is the kind of change which could likely induce dependencies
and thus not be easily cherry-picked out.
eliminate autogeneration of Workaround.hs
-----------------------------------------
Cool! I'm glad to see all this cleverness go away.
For what it's worth, I might have split this into at least two
patches, for example one that deletes workarounds and one that
just rearranges them.
I suspect removing the openFd workaround will break GHC 6.8, but
otherwise I think this is fine (and I don't think the System.Posix
imports are a cause for concern)
Here are the three types of changes I noticed
Moved to System.Posix.Files:
> -# WORKAROUND_createLink
> -# WORKAROUND_fileModes
Moved to the new Workaround module:
> -# WORKAROUND_POSIXSIGNALS(IMPORTS)
> -# WORKAROUND_executable
> -# WORKAROUND_renameFile
> -# WORKAROUND_getCurrentDirectory
Deleted:
> -# WORKAROUND_bracketOnError
> -# WORKAROUND_createDirectoryIfMissing
> -# WORKAROUND_openFd
Note that the openFd stuff has some different code for GHC 6.6
and 6.8, so I suspect that deleting this breaks the build on GHC 6.8
> hunk ./src/Autoconf.lhs.in 74
> +#ifdef WIN32
> +path_separator = '\\'
> +#else
> +path_separator = '/'
> +#endif
Due to 64 bit Windows, it may be a good idea to rename it our WIN32
ifdef to just WINDOWS and darcs mv src/win32 as well.
> hunk ./src/Darcs/External.hs 47
> +import System.Posix.Files ( createLink )
> +import System.Directory ( createDirectoryIfMissing )
For the interested, this import is ok because we have our own
System.Posix.Files in src/win32
> addfile ./src/Workaround.hs
> hunk ./src/Workaround.hs 1
> +#ifndef HAVE_SIGNALS
> +-- Dummy implementation of POSIX signals
> +data Handler = Default | Ignore | Catch (IO ())
> +type Signal = Int
Maybe we should create a System.Posix.Signals in src/win32?
> +#ifdef WIN32
> +renameFile :: FilePath -> FilePath -> IO ()
> +renameFile old new = Control.Exception.block $
> + do System.Directory.removeFile new
> + `System.IO.Error.catch`
> + (\e -> if System.IO.Error.isDoesNotExistError e
> + then return ()
> + else System.IO.Error.ioError e)
> + System.Directory.renameFile old new
> +
Before we actually tested for this, now we just assume it's buggy on
Windows and only on Windows. For that matter I wonder if the function
really is still buggy. It seems like we should document which version
of GHC / the directory package we see this bug on. [That said, the only
version of directory on hackage is only 1.0.0.0]
On Windows:
> +setExecutable :: FilePath -> Bool -> IO ()
> +setExecutable _ _ = return ()
Elsewhere:
> +setExecutable :: FilePath -> Bool -> IO ()
> +setExecutable f ex =
> + do st <- getFileStatus f
> + umask <- setFileCreationMask 0
> + setFileCreationMask umask
> + let rw = fileMode st .&.
> + (ownerReadMode .|. ownerWriteMode .|.
> + groupReadMode .|. groupWriteMode .|.
> + otherReadMode .|. otherWriteMode)
> + total = if ex then rw .|.
> + ((ownerExecuteMode .|. groupExecuteMode .|.
> otherExecuteMode)
> + .&. complement umask)
> + else rw
> + setFileMode f total
Unchanged. But maybe this function belows in Darcs.Utils so that we can
reserve the Workarounds module for functions that we are trying to use
from libraries.
make various autoconf simplifications.
--------------------------------------
NB. I have rearranged these changes to make my reply simpler
Some people might object to datadir being hard-coded for the man page,
but it doesn't seem like too huge a deal.
> hunk ./src/Autoconf.lhs.in 8
> -darcsconfdir = "@sysconfdir@/darcs"
> -libexecdir = "@libexecdir@"
Removed because unused.
> -dnl Look for a suitable diff command
> -
> -AC_CHECK_PROGS(DIFF, [gdiff gnudiff diff])
>
> hunk ./src/Autoconf.lhs.in 56
> -{-# INLINE diff_program #-}
> -diff_program :: String
> -diff_program = "@DIFF@"
>
> hunk ./src/Darcs/Commands/Diff.lhs 139
> - Right (diff_program, ("-rN":get_diff_opts opts++[f1,f2]))
> + Right ("diff", ("-rN":get_diff_opts opts++[f1,f2]))
Before this used to be diff, gdiff or gnudiff depending on what system
darcs was built on. I guess without confirming that non-GNU diff is
quite happy with the -rN switches
> hunk ./src/Autoconf.lhs.in 8
> -{-# INLINE datadir #-}
> -datadir :: String
> -datadir = "@datadir@"
> hunk ./src/darcsman.hs 93
> - datadir++"/doc/darcs/manual/index.html. To easily get more specific
> help\n"++
> + "/usr/share/doc/darcs/manual/index.html. To easily get more specific
> help\n"++
I wonder if people might object to this being hard-coded
> hunk ./src/darcsman.hs 19
> import Time
BTW, it looks like this Haskell98 import ought to be modernised
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgpgS3ErWCUow.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
