On Sun, Sep 21, 2008 at 8:12 PM, Eric Kow <[EMAIL PROTECTED]> wrote:
> 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)
No, it's tested and works on both ghc 6.6 and 6.8. We never used
Workaround.openFd.
> 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
No, as I mentioned above, it's never used.
>> 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.
We could, but as far as I know, the API is still called win32. Does
ghc support the creation of 64 bit windows code?
>> 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?
Not a bad idea, but probably it should be a secondary refactor.
>> +#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]
It's really just a bug in the documentation, I think, since it's not
possible to atomically replace a file under windows. So there's
definitely always going to be a bug on windows until the documentation
changes, and it seems highly unlikely that ghc will decide to change
bugs by switching to this workaround. If they do, we can implement a
test at that point.
Incidentally, the same bug exists on POSIX systems that use windows
file systems, so maybe the answer tis to avoid ifdeffing and to catch
the already-exists exception and remove the file and try again.
> 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.
Sure, I'd love to eliminate Workaround.
> 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.
It's a minor regression, but seems worth the simplification.
>> 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
This may be a relevant regression on BSD systems.
David
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
