Re: [tor-dev] PRELIMINARY: [PATCH] Adapt to changes in 'GHC.Handle'.

[Nikita Karetnikov]

 It would be a bit sad if it fell to me to reviewing the haskell code
 alone, since I don't really know Haskell very well.

If so, don't spend too much time on the preliminary patches.  I think
that quick review is enough at this point.

 I get that this is required for more warnings, though, and I think
 it's okay to turn it off temporarily.

Yes, '-Werror' converts all warnings into errors [1], but it also
hides useful messages sometimes.  '-Werror' is suitable for
development but not release.  [...]  The package will break silently
as soon as the next version of ghc adds a new warning, which generally
does happen every major release. [2]

 Is there a conditional build thing where we can get -Werror when
 developing, and turn it off for hackage?

Yep, take a look at [3] and [4].  (I'll ask someone about Hackage.)

What do you think about an attached patch?  We can add other flags if
it's needed.

Example ('git pull' and 'git am' were omitted):

# ghc --version
The Glorious Glasgow Haskell Compilation System, version 6.12.1
# cabal --version
cabal-install version 1.16.0.2
using version 1.16.0 of the Cabal library
# ./Setup.lhs configure --user

[...]

Configuring TorDNSEL-0.1.1...
# ./Setup.lhs build

[...]

dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Util.hs:1:11:
Warning: -#include is deprecated: No longer has any effect
[ 4 of 39] Compiling TorDNSEL.Util( 
dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Util.hs, 
dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Util.o )

src/TorDNSEL/Util.hsc:143:23:
Module `GHC.Handle' does not export `fillReadBuffer'

src/TorDNSEL/Util.hsc:143:39:
Module `GHC.Handle' does not export `readCharFromBuffer'

src/TorDNSEL/Util.hsc:145:26:
Module `GHC.IOBase' does not export `Buffer(..)'

# ./Setup.lhs configure --user -f devel

[...]

# ./Setup.lhs build

[...]

dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Main.hs:3:11:
Warning: -#include is deprecated: No longer has any effect

no location info: 
Failing due to -Werror.

I don't think there is a need to push this patch right now.  There
might be other issues (for instance, incorrect build dependencies.)

 Hm.  I don't understand how the hgetLineN implementation can work.  It
 looks like it reads N bytes unconditionally, then looks for the EOL
 marker in them , and returns everything before the EOL marker... but
 what does it do with everything after the EOL marker?  It appears to
 me that if the EOL marker is not right at the end of the N bytes,
 those bytes would get lost.

Right, should it work differently?  (Again, I haven't inspected
'hGetLine' yet.)

The current version allows to estimate a worst case scenario because
it reads N bytes unconditionally.  But it might be better to look for
the EOL marker first, then check the number of bytes.  What do you
think?

 Am I missing something there?  Do the extra bytes get put back somehow?

No.  Here is an example:

# cat  Main.hs
module Main where
import Data.ByteString
import qualified Data.ByteString.Char8 as B
import System.IO

-- | Read @n@ bytes from @handle@; strip @eol@ (e.g., @'B.pack' \r\n@)
-- and everything after it.
hGetLineN :: Handle - ByteString - Int - IO ByteString
hGetLineN handle eol n = do
  hSetBuffering handle LineBuffering
  bStr - B.hGet handle n
  return $ fst $ B.breakSubstring eol bStr

main = do
  handle - openFile test.txt ReadMode
  hGetLineN handle (B.pack \n) 42

# echo -e foo\nbar\nbaz  test.txt
# runhaskell Main
foo

Does it answer your question?

(FYI: a chapter about I/O [5], an introduction to bytestrings [6], and
a search engine [7].)

 If you apply both patches (with GHC 7.6.3), the following errors will
 appear:

 [ 3 of 39] Compiling TorDNSEL.Compat.Exception ( 
 src/TorDNSEL/Compat/Exception.hs, 
 dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Compat/Exception.o ) 
 [dist/build/autogen/cabal_macros.h changed]
 [ 4 of 39] Compiling TorDNSEL.System.Timeout ( 
 src/TorDNSEL/System/Timeout.hs, 
 dist/build/tordnsel/tordnsel-tmp/TorDNSEL/System/Timeout.o )

 I don't see the errors there... did they go to stderr or something?

I was a bit sleepy and messed it up.  Here is the right output:

src/TorDNSEL/System/Timeout.hs:56:47:
Module `TorDNSEL.Compat.Exception' does not export `throwDynTo'

src/TorDNSEL/System/Timeout.hs:56:59:
Module `TorDNSEL.Compat.Exception' does not export `dynExceptions'

 Seems reasonable.  It might also be good to have a publicly git
 repository somewhere where you work on the branch, that can store all
 of the preliminary repositories.

Can I create a new branch here [8]?  If so, should I also send patches
and comments to this list, or will it only annoy everyone?

[1] http://www.haskell.org/ghc/docs/7.6.3/html/users_guide/flag-reference.html
[2] http://hackage.haskell.org/trac/hackage/ticket/191
[3] 
http://www.haskell.org/cabal/users-guide/developing-packages.html#configurations
[4] 
http://www.haskell.org/cabal/users-guide/installing-packages.html#controlling-flag-assignments
[5] 

Re: [tor-dev] PRELIMINARY: [PATCH] Adapt to changes in 'GHC.Handle'. (was: Haskell packages?)

[Nick Mathewson]

On Wed, Jun 19, 2013 at 12:12 AM, Nikita Karetnikov
nik...@karetnikov.org wrote:
[..]

Hi, and thanks for the patches!

Does anyone else on this list know Haskell at all well?  It would be a
bit sad if it fell to me to reviewing the haskell code alone, since I
don't really know Haskell very well.

I expect that some of my questions below will display my haskell
ignorance; please forgive that. :)

   * The first patch removes '-Werror' from 'tordnsel.cabal'.  I guess
 that '-Werror' shouldn't be there because Hackage prevents people
 uploading packages with '-Werror' in their '.cabal' file [2].  But
 I'd prefer to fix other 'cabal'-related warnings before pushing, for
 instance:

 Warning: Instead of 'ghc-options: -DVERSION=0.1.1-dev' use 'cpp-options:
 -DVERSION=0.1.1-dev'

 This patch is needed to produce more verbose error messages on
 'cabal build'.

Hm. I'm okay removing a -Werror , but... I am not okay with having any
warnings in the code, really.  I get that this is required for more
warnings, though, and I think it's okay to turn it off temporarily.

  Is there a conditional build thing where we can get -Werror when
developing, and turn it off for hackage?

   * The second patch tries to adapt to changes in 'GHC.Handle'.

Hm.  I don't understand how the hgetLineN implementation can work.  It
looks like it reads N bytes unconditionally, then looks for the EOL
marker in them , and returns everything before the EOL marker... but
what does it do with everything after the EOL marker?  It appears to
me that if the EOL marker is not right at the end of the N bytes,
those bytes would get lost.

Am I missing something there?  Do the extra bytes get put back somehow?

 If you apply both patches (with GHC 7.6.3), the following errors will
 appear:

 [ 3 of 39] Compiling TorDNSEL.Compat.Exception ( 
 src/TorDNSEL/Compat/Exception.hs, 
 dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Compat/Exception.o ) 
 [dist/build/autogen/cabal_macros.h changed]
 [ 4 of 39] Compiling TorDNSEL.System.Timeout ( 
 src/TorDNSEL/System/Timeout.hs, 
 dist/build/tordnsel/tordnsel-tmp/TorDNSEL/System/Timeout.o )

I don't see the errors there... did they go to stderr or something?

 I don't know how many things I'll have to change to build TorDNSEL.
 I know about changes in 'base', exceptions, and 'binary'.  For
 example:

   - Compare [3] and [4];
   - This version [5] doesn't have 'lookAhead', which is used in
 TorDNSEL.

 ('tordnsel.cabal' should be adjusted accordingly, by the way.)

 My plan is to build TorDNSEL first.  I assume it'll take a lot of time.
 So I decided not to look through 'getRequest', 'hGetLine',
 'startSocketReader', and other affected functions at this point.
 Basically, 'hGetLineN' is based on a docstring of 'hGetLine' and its
 type declaration.

 I'd like to send more preliminary patches if you don't mind.  That
 will help to ensure that I'm on the right track.  Also, it's possible
 that someone will be able to spot bugs at preliminary stages.

Seems reasonable.  It might also be good to have a publicly git
repository somewhere where you work on the branch, that can store all
of the preliminary repositories.

 I'll inspect each preliminary patch when I build the program.

 What do you think?

It seems like a plan; get it to build is a good and necessary first step.

best wishes,
-- 
Nick
___
tor-dev mailing list
tor-dev@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev

Re: [tor-dev] PRELIMINARY: [PATCH] Adapt to changes in 'GHC.Handle'.

[Nikita Karetnikov]

 Hm, I can't build it with GHC 6.10.4.  'cabal build' returns the
 following:

 src/TorDNSEL/Compat/Exception.hs:23:7:
 Could not find module `Control.OldException':
   it is a member of the hidden package `base'
   Use -v to see a list of the files searched for.

 (Oh, I guess I've just found a workaround.  I'll try it later.)

Indeed.  It was necessary to run 'ghc-pkg hide base-3.0.3.1' and
'ghc-pkg expose base-4.1.0.0'.  After that these commands did the
trick: './Setup.lhs configure --user' and './Setup.lhs build'.

The '--user' option was needed because 'cabal install foo' installs a
package locally by default. [1]  (Also, see 'ghc-pkg list --user'.)

[1] 
http://stackoverflow.com/questions/4641684/ghc-cant-find-my-cabal-installed-packages/17170760#17170760


pgp_O4ZwUb4wS.pgp
Description: PGP signature
___
tor-dev mailing list
tor-dev@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev