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

[Nick Mathewson <nickm@xxxxxxxxxxxx>]

On Wed, Jun 19, 2013 at 12:12 AM, Nikita Karetnikov
<nikita@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev