[darcs-users] darcs patch: Replace Autoconf.hs with consistent use of CPP.

Petr Rockai me at mornfall.net
Thu Feb 19 14:17:23 UTC 2009


Hi,

Trent W.Buck <trentbuck at gmail.com> writes:
> This works a bit better.
promised review below.

> Thu Feb 19 17:45:16 EST 2009  Trent W. Buck <trentbuck at gmail.com>
>   * Replace Autoconf.hs with consistent use of CPP.
>   
>   It's annoying how recording a patch causes a bunch of unrelated
>   modules like SHA1 to be recompiled, simply because they use *other*
>   variables exported by Autoconf.
>   
>   Since everything is now available via processor definitions, and these
>   definitions are already used in several places, let's just use them
>   everywhere and remove Autoconf entirely.
In general, I'm in favour of such a change. Even if for no other reason than
saving compile times, I believe this is worthwhile.

There are some pros:
- the conditionally compiled code is more clearly delineated
- conditionals are so fugly that most people will be automagically compelled to
  reduce conditional compilation

and some cons:

- it makes it easier to accidentally break compilation of other branches than
  your own (this might also be considered an advantage, since even if you fixed
  the compilation because compiler forced you to, you would not test your
  changes anyway)

NB. Comments inline.

Replace Autoconf.hs with consistent use of CPP.
-----------------------------------------------
>  It's annoying how recording a patch causes a bunch of unrelated
>  modules like SHA1 to be recompiled, simply because they use *other*
>  variables exported by Autoconf.
>  
>  Since everything is now available via processor definitions, and these
>  definitions are already used in several places, let's just use them
>  everywhere and remove Autoconf entirely.
> ] hunk ./configure.ac 24
>  dnl The following is now the authoritative location of the version
>  dnl number. All references to the darcs version number should
>  dnl originate here. From Haskell this is easily done by importing
> -dnl the module "Autoconf ( darcs_version )".
> +dnl the module "ThisVersion ( darcs_version )".
>  dnl
>  dnl The version string must not contain any spaces. The version
>  dnl state will automatically be set to any of:
Documentation update. Clear.

> hunk ./configure.ac 681
>  
>      Build Manual    = ${BUILDDOC+yes (}${BUILDDOC:-no}${BUILDDOC+)}
>  
> -If you want to adjust any of these values, edit autoconf.mk and
> -src/Autoconf.hs -- or run configure with appropriate settings.
> +If you want to adjust any of these values, edit autoconf.mk,
> +or run configure with appropriate settings.
>  
>  [EOF]
>  
> hunk ./darcs.cabal 287
>                      Workaround
>  
>    other-modules:    ThisVersion
> -                    Autoconf
>  
>    c-sources:        src/atomic_create.c
>                      src/fpstring.c
Ack, fix up darcs.cabal.

> hunk ./src/Autoconf.hs 1
[cut]
> rmfile ./src/Autoconf.hs
All clear.

> hunk ./src/Darcs/Bug.hs
> -import Autoconf( darcs_version )
> +import ThisVersion ( darcs_version )
Ack.

> hunk ./src/Darcs/Commands/Help.lhs 27
> -import Autoconf( darcs_version )
> +import ThisVersion ( darcs_version )
Ack.

> hunk ./src/Darcs/Commands/Send.lhs 32
>  import Control.Monad ( when, unless, forM_ )
>  import Data.Maybe ( isJust, isNothing )
>  
> -import Autoconf ( have_HTTP )
>  import Darcs.Commands ( DarcsCommand(..) )
>  import Darcs.Arguments ( DarcsFlag( EditDescription, LogFile, RmLogFile,
>                                      Target, OutputAutoName, Output, Context,
> hunk ./src/Darcs/Commands/Send.lhs 333
>      ts -> do announce_recipients ts
>               return ts
>      where the_targets = collect_targets opts
> -          check_post | have_HTTP =
> -                         do p <- ((readPost . BC.unpack) `fmap`
> -                                  fetchFilePS (prefsUrl the_remote_repo++"/post")
> -                                  (MaxAge 600)) `catchall` return []
> -                            emails <- who_to_email
> -                            return (p++emails)
> -                     | otherwise = who_to_email
> +#ifdef have_HTTP
Should this be HAVE_HTTP instead of have_HTTP?

> +          check_post = do p <- ((readPost . BC.unpack) `fmap`
> +                                fetchFilePS (prefsUrl the_remote_repo++"/post")
> +                                (MaxAge 600)) `catchall` return []
> +                          emails <- who_to_email
> +                          return (p++emails)
> +          readPost p = map pp (lines p) where
> +            pp ('m':'a':'i':'l':'t':'o':':':s) = SendMail s
> +            pp s = Post s
> +#else
> +          check_post = who_to_email
> +#endif
Ack.

>            who_to_email =
>                do email <- (BC.unpack `fmap`
>                             fetchFilePS (prefsUrl the_remote_repo++"/email")
> hunk ./src/Darcs/Commands/Send.lhs 352
>                            `catchall` return ""
>                   if '@' `elem` email then return . map SendMail $ lines email
>                                       else return []
> -          readPost p = map pp (lines p) where
> -            pp ('m':'a':'i':'l':'t':'o':':':s) = SendMail s
> -            pp s = Post s
Moved above.

>            putInfoLn s = unless (Quiet `elem` opts) $ putStrLn s
>            announce_recipients emails =
>              let pn (SendMail s) = s
> hunk ./src/Darcs/External.hs 48
>  import Foreign.C ( CChar, CInt )
>  import Foreign.Ptr ( Ptr )
>  import Foreign.Marshal.Alloc (allocaBytes)
> -import Autoconf ( use_color )
Ack.

>  #endif
>  import System.Posix.Files ( createLink )
>  import System.Directory ( createDirectoryIfMissing )
> hunk ./src/Darcs/External.hs 68
>  import Darcs.Lock ( withTemp, withOpenTemp, tempdir_loc, canonFilename, writeDocBinFile,
>                      removeFileMayNotExist, )
>  import CommandLine ( parseCmd, addUrlencoded )
> -import Autoconf ( have_libcurl, have_libwww, have_HTTP, have_mapi, darcs_version )
> +import ThisVersion ( darcs_version )
Ack.

> +#if defined(HAVE_LIBWWW) || defined(HAVE_LIBCURL) || defined(HAVE_HTTP)
>  import URL ( copyUrl, copyUrlFirst, waitUrl )
> hunk ./src/Darcs/External.hs 71
> +#endif
Conditional imports.

>  import Ssh ( getSSH, copySSH, copySSHs, SSHCmd(..) )
>  import URL ( Cachable(..) )
>  import Exec ( exec, Redirect(..), withoutNonBlock )
> hunk ./src/Darcs/External.hs 222
>               `catch` \_ -> return Nothing
>  
>  speculateRemote :: String -> FilePath -> IO () -- speculations are always Cachable
> +#if defined(HAVE_LIBWWW) || defined(HAVE_LIBCURL) || defined(HAVE_HTTP)
>  speculateRemote u v =
>      do maybeget <- maybeURLCmd "GET" u
>         case maybeget of
> hunk ./src/Darcs/External.hs 227
>           Just _ -> return () -- can't pipeline these
> -         Nothing -> if have_libwww || have_libcurl || have_HTTP
> -                    then copyUrl u v Cachable
> -                    else return ()
> +         Nothing -> copyUrl u v Cachable
> +#else
> +speculateRemote u _ = maybeURLCmd "GET" u >> return ()
Same as maybeURLCmd "GET" u (without >> return()).

> +#endif
>  
>  copyRemote :: String -> FilePath -> Cachable -> IO ()
>  copyRemote u v cache =
> hunk ./src/Darcs/External.hs 244
>                    fail $ "(" ++ get ++ ") failed to fetch: " ++ u
>  
>  copyRemoteNormal :: String -> FilePath -> Cachable -> IO ()
> -copyRemoteNormal u v cache = if have_libwww || have_libcurl || have_HTTP
> -                             then copyUrlFirst u v cache >> waitUrl u
> -                             else copyRemoteCmd u v
> +#if defined(HAVE_LIBWWW) || defined(HAVE_LIBCURL) || defined(HAVE_HTTP)
> +copyRemoteNormal u v cache = copyUrlFirst u v cache >> waitUrl u
> +#else
> +copyRemoteNormal u v _ = copyRemoteCmd u v
> +#endif
Looks OK.

> hunk ./src/Darcs/External.hs 297
>      upto n (h : t) = h : (upto (n - 1) t)
>  
>  copyRemotesNormal :: String -> [String] -> FilePath -> Cachable -> IO()
> +#if defined(HAVE_LIBWWW) || defined(HAVE_LIBCURL) || defined(HAVE_HTTP)
>  copyRemotesNormal u ns d cache =
> hunk ./src/Darcs/External.hs 299
> -    if have_libwww || have_libcurl || have_HTTP
> -    then do mapM_ (\n -> copyUrl (u++"/"++n) (d++"/"++n) cache) ns
> -            doWithPatches (\n -> waitUrl (u++"/"++n)) ns
> -    else wgetRemotes u ns d
> +    do mapM_ (\n -> copyUrl (u++"/"++n) (d++"/"++n) cache) ns
> +       doWithPatches (\n -> waitUrl (u++"/"++n)) ns
> +#else
> +copyRemotesNormal u ns d _ = wgetRemotes u ns d
> +#endif
Look OK.

> -   else if have_mapi then do
> +#ifdef HAVE_MAPI
> +   else do
>       r <- withCString t $ \tp ->
>             withCString f $ \fp ->
>              withCString cc $ \ccp ->
> hunk ./src/Darcs/External.hs 472
>                 cfn <- canonFilename fn
>                 withCString cfn $ \pcfn ->
>                  c_send_email fp tp ccp sp nullPtr pcfn
> -     when (r /= 0) $ fail ("failed to send mail to: " ++ t)
> +         when (r /= 0) $ fail ("failed to send mail to: " ++ t)
> +#else
>     else fail $ "no mail facility (sendmail or mapi) located at configure time!"
> hunk ./src/Darcs/External.hs 475
> +#endif
>    where addressOnly a =
>            case dropWhile (/= '<') a of
>            ('<':a2) -> takeWhile (/= '>') a2
> hunk ./src/Darcs/External.hs 488
>  resendEmail "" _ _ = return ()
>  resendEmail t scmd body = do
>    use_sendmail <- have_sendmail
> -  case (use_sendmail || scmd /= "", have_mapi) of
> -   (True, _) -> do
> +  if use_sendmail || scmd /= ""
> +   then do
>      withOpenTemp $ \(h,fn) -> do
>       hPutStrLn h $ "To: "++ t
>       hPutStrLn h $ find_from (linesPS body)
> hunk ./src/Darcs/External.hs 499
>       let ftable = [('t',t)]
>       r <-  execSendmail ftable scmd fn
>       when (r /= ExitSuccess) $ fail ("failed to send mail to: " ++ t)
> -   (_, True) -> fail "Don't know how to resend email with MAPI"
> -   _ -> fail $ "no mail facility (sendmail or mapi) located at configure time (use the sendmail-command option)!"
> +   else
> +#ifdef HAVE_MAPI
> +    fail "Don't know how to resend email with MAPI"
> +#else
> +    fail "no mail facility (sendmail or mapi) located at configure time (use the sendmail-command option)!"
> +#endif
Looks superficially OK although I haven't tried hard here.

>    where br            = BC.pack "\r"
>          darcsurl      = BC.pack "DarcsURL:"
>          content       = BC.pack "Content-"
> hunk ./src/Darcs/External.hs 705
>  termioBufSize :: Int
>  termioBufSize = 4096
>  
> -getTermNColors = if not use_color
> -                 then return (-1)
> -                 else do term <- getEnv "TERM"
> -                         allocaBytes termioBufSize (getTermNColorsImpl term)
> -                      `catch` \_ -> return (-1)
> +#ifndef USE_COLOR
> +getTermNColors = return (-1)
> +#else
> +getTermNColors = do term <- getEnv "TERM"
> +                    allocaBytes termioBufSize (getTermNColorsImpl term)
> +                 `catch` \_ -> return (-1)
> +#endif
Ack.

>  getTermNColorsImpl :: String -> Ptr CChar -> IO Int
>  getTermNColorsImpl term buf = do rc <- withCString term $
> hunk ./src/Darcs/RepoPath.hs 35
>  import Control.Exception ( bracket )
>  
>  import Darcs.URL ( is_absolute, is_relative, is_ssh_nopath )
> -import Autoconf ( path_separator )
Ack.

>  import qualified Workaround ( getCurrentDirectory )
>  import qualified System.Directory ( setCurrentDirectory )
>  import System.Directory ( doesDirectoryExist )
> hunk ./src/Darcs/RepoPath.hs 184
>      show (AbsP a) = show a
>      show (RmtP r) = show r
>  
> +-- | When mapped over characters in a path, this function normalizes
> +-- the path separator to Unix style (slash, not backslash).  This only
> +-- affects Windows systems.
>  cleanup :: Char -> Char
> hunk ./src/Darcs/RepoPath.hs 188
> -cleanup '\\' | path_separator == '\\' = '/'
> +#ifdef WIN32
> +cleanup '\\' = '/'
> +#endif
>  cleanup c = c
Bonus haddock.

>  norm_slashes :: String -> String
> hunk ./src/SHA1.hs 18
>  -- the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>  -- Boston, MA 02110-1301, USA.
>  
> -{-# OPTIONS_GHC -fno-warn-name-shadowing #-}
> +{-# OPTIONS_GHC -fno-warn-name-shadowing -cpp #-}
> +{-# LANGUAGE CPP #-}
Did we miss those in other places? (I guess no if it compiles for you...)

>  -- {-# OPTIONS_GHC -fglasgow-exts -fno-warn-name-shadowing #-}
>  -- -fglasgow-exts needed for nasty hack below
> hunk ./src/SHA1.hs 27
>  -- name shadowing disabled because a,b,c,d,e are shadowed loads in step 4
>  module SHA1 (sha1PS) where
>  
> -import Autoconf (big_endian)
>  import ByteStringUtils (unsafeWithInternals)
>  import qualified Data.ByteString as B (ByteString, pack, length, concat)
>  
> hunk ./src/SHA1.hs 30
> -import Control.Monad (unless)
>  import Data.Char (intToDigit)
>  import Data.Bits (xor, (.&.), (.|.), complement, rotateL, shiftL, shiftR)
>  import Data.Word (Word8, Word32)
> hunk ./src/SHA1.hs 48
>         abcde' = unsafePerformIO
>                $ unsafeWithInternals s1_2 (\ptr len ->
>                      do let ptr' = castPtr ptr
> -                       unless big_endian $ fiddle_endianness ptr' len
> +#ifndef BIGENDIAN
> +                       fiddle_endianness ptr' len
> +#endif
>                         sha1_step_4_main abcde ptr' len)
>         s5 = sha1_step_5_display abcde'
>  
> hunk ./src/darcs.hs 32
>  import Darcs.RunCommand ( run_the_command )
>  import Darcs.Flags ( DarcsFlag(Verbose) )
>  import Darcs.Commands.Help ( help_cmd, list_available_commands, print_version )
> -import Autoconf( darcs_version )
> +import ThisVersion ( darcs_version )
>  import Darcs.SignalHandler ( withSignalsHandled )
>  import Context ( context )
>  import Darcs.Global ( with_atexit )
> hunk ./src/preproc.hs 14
>                          extract_commands )
>  import Darcs.Arguments ( options_latex )
>  import Darcs.Commands.Help ( command_control_list )
> -import Autoconf ( darcs_version )
> +import ThisVersion ( darcs_version )
>  
>  the_commands :: [DarcsCommand]
>  the_commands = extract_commands command_control_list
All ack.

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation


More information about the darcs-users mailing list