Hi Nikita! Nikita Karetnikov:

I'd like to improve my Haskell skills. Are there any opportunities?

I've been told there is at least one project that uses Haskell, which is not maintained. (For example, this page [1] mentions TorDNSEL, which was replaced by TorBEL.)

TorDNSEL is providing an important service to the Tor community right now. It should be replaced by TorBEL at some point. But the later is not ready. In order to have TorDNSEL work on Debian Squeeze, I did some small changes to make it uses Control.OldException. But this does not work anymore on GHC 7.4. The later is the version currently in Debian Wheezy. If you wish to make weasel (one of Tor all mighty sysadmins) happy you could update TorDNSEL to the new APIs and make sure that it works on Debian Wheezy. How do you feel about it? Even if my Haskell is rusty, I'd be happy to have a look at your changes, if you want/need some reviews. :) -- Lunar <lunar@torproject.org>

There is exactly one such program: https://www.torproject.org/tordnsel/dist/

On the bright side (for you, not for us) TorBEL never replaced it. We still use TorDNSEL to generate the exit-addresses files: https://exitlist.torproject.org/

I've found out that TorDNSEL is in the public domain (see [1]). It's OK, but I'm afraid that my (future) work will be proprietarized [2]. I'd prefer to relicense the whole thing under the GPLv3+ [3]. So TorDNSEL will stay free software [2]. What do you think? If you are OK with that, I'll talk to the FSF [4] to ensure that everything is fine. It shouldn't be too hard, but I have some questions. For instance, will it be OK to remove copyright notices from sources because the license [1] says that "the 'Dedicator' [...] hereby dedicates whatever copyright the dedicators holds in the work of authorship [...] to the public domain"? [1] https://gitweb.torproject.org/tordnsel.git/blob/HEAD:/LICENSE [2] https://www.gnu.org/philosophy/free-sw [3] https://www.gnu.org/licenses/gpl.html [4] https://www.fsf.org/

On Sat, 08 Jun 2013 19:58:06 +0400 Nikita Karetnikov <nikita@karetnikov.org> wrote:

I've found out that TorDNSEL is in the public domain (see [1]).

The official Tordnsel will remain in the public domain. You're welcome for fork it of course. However, if you want to commit back to us, please don't change the licensing. -- Andrew http://tpo.is/contact pgp 0x6B4D6475

On Wed, Jun 19, 2013 at 12:12 AM, Nikita Karetnikov <nikita@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

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#configurat... [4] http://www.haskell.org/cabal/users-guide/installing-packages.html#controllin... [5] http://learnyouahaskell.com/input-and-output#hello-world [6] https://www.fpcomplete.com/school/pick-of-the-week/bytestring-bits-and-piece... [7] http://www.haskell.org/hoogle/ [8] https://gitweb.torproject.org/tordnsel.git

We use the main repositories only for released versions. FOr development branches, we use pesonal repositories. Most people find that it's more convenient to use a service like github or gitorious or something to host their own branches, and then ask for review and merging when the branches are ready for review and merging.

It's okay to send stuff to the list, but linking to a public branch and listing the commit IDs usually works as well as sending patches.

I don't have a server that I can physically control, and I'm trying to avoid third party services. Most of them use freedom-denying software. Moreover, companies might decide to shut them down for some reason. Imagine that I also won't be able to provde the needed code in that case. That would be very frustrating. Lists are usually mirrored, plus subscribers might have local copies. That's why I'd prefer to send patches to the list. If anyone finds that inconvenient, I'll try to find a workaround. I'm attaching two preliminary patches. I'm more confident about them than the previous ones. Still, I can't build the program or run the tests, so you shouldn't expect them to work. '0002-Remove-a-redundant-instance-declaration.patch' is self-explanatory. (I set 'network' to '>=2.4.0.0' because it's the first version that derives 'Ord'.) If you want to test the other patch, copy the functions to 'Test.hs' and run the following: $ ghc --version The Glorious Glasgow Haskell Compilation System, version 7.6.3 $ ghci Test.hs *Main> import Data.Dynamic (toDyn, fromDynamic) *Main Data.Dynamic> E.tryJust syncExceptions $ E.throwIO E.DivideByZero Left divide by zero *Main Data.Dynamic> E.tryJust syncExceptions $ E.throwIO E.StackOverflow *** Exception: stack overflow *Main Data.Dynamic> ignoreJust syncExceptions $ E.throwIO E.StackOverflow *** Exception: stack overflow *Main Data.Dynamic> ignoreJust syncExceptions $ E.throwIO E.DivideByZero *Main Data.Dynamic> showException [fromDynamic] $ E.toException $ toDyn "foo" "foo" *Main Data.Dynamic> showException [fromDynamic] $ E.toException E.DivideByZero "divide by zero" *Main Data.Dynamic> showException [fromDynamic, (return . show)] $ E.toException $ toDyn "foo" "foo" *Main Data.Dynamic> showException [fromDynamic, (return . show)] $ E.toException $ E.DivideByZero "divide by zero" If you want to test the original code (using GHC 6.10.4), you should adjust some functions: *Main> showException [fromDynamic] $ E.DynException $ toDyn "foo" "foo" *Main> showException [fromDynamic] $ E.ArithException E.DivideByZero "divide by zero" *Main> showException [fromDynamic, (return . show)] $ E.DynException $ toDyn "foo" "foo" *Main> showException [fromDynamic, (return . show)] $ E.ArithException E.DivideByZero "divide by zero" Did I forget any corner cases? I tried to write tests for the above, but it didn't work well with HUnit. Should I try QuickCheck? I guess we don't want to depend on two testing libraries, do we? If you apply the attached patches on top of [1] and [2], you should get the following: $ ./Setup.lhs configure --user $ ./Setup.lhs build [...] [ 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:1: Warning: Module `GHC.IOBase' is deprecated: use GHC.IO instead src/TorDNSEL/Util.hsc:380:47: Warning: In the use of `B.findSubstrings' (imported from Data.ByteString.Char8, but defined in Data.ByteString): Deprecated: "findSubstrings is deprecated in favour of breakSubstring." [ 7 of 39] Compiling TorDNSEL.Random ( src/TorDNSEL/Random.hs, dist/build/tordnsel/tordnsel-tmp/TorDNSEL/Random.o ) src/TorDNSEL/Random.hs:39:1: Unacceptable argument type in foreign declaration: CInt When checking declaration: foreign import ccall unsafe "static openssl/rand.h RAND_bytes" c_RAND_bytes :: Ptr Word8 -> CInt -> IO CInt src/TorDNSEL/Random.hs:39:1: Unacceptable result type in foreign declaration: IO CInt When checking declaration: foreign import ccall unsafe "static openssl/rand.h RAND_bytes" c_RAND_bytes :: Ptr Word8 -> CInt -> IO CInt src/TorDNSEL/Random.hs:59:1: Unacceptable argument type in foreign declaration: CInt When checking declaration: foreign import ccall unsafe "static openssl/rand.h RAND_seed" c_RAND_seed :: Ptr Word8 -> CInt -> IO () src/TorDNSEL/Random.hs:65:1: Unacceptable result type in foreign declaration: IO CInt When checking declaration: foreign import ccall unsafe "static openssl/rand.h RAND_status" c_RAND_status :: IO CInt It's also necessary to fix exceptions in other modules (run 'grep -r Exception src'). Any comments? [1] http://lists.torproject.org/pipermail/tor-dev/attachments/20130622/3e84acaf/... [2] http://lists.torproject.org/pipermail/tor-dev/attachments/20130619/34418869/...

Hi Nikita, Sorry for being slow to get to your patches. They are much appreciated and even needed now that checks.tpo has seen repetitive failures. An overall comment: I am unconvinced about commit messages that details obvious changes for each impacted file. For example:

* src/TorDNSEL/Util.hsc: Use 'GHC.IO.Handle.Types' and 'GHC.IORef' instead of 'GHC.IOBase'. (splitByDelimiter): Remove it.

Both are completly obvious by looking at the commit diff, so there is no added value in such message. This single commit should probably be split in two, one saying "update imports to the new locations of internal GHC functions" and another one saying "remove the unused splitByDelimiter function". I am not sure I completly understand all the intents, so I might have misunderstood the intents. Nikita Karetnikov:

The first one deals with this issue: "GHC was recently changed to not allow you to use newtypes in FFI imports unless the constructor of the newtype is in scope."

This one looks good.

The second patch removes a redundant function and adjusts some imports. (I wrote a version of 'splitByDelimiter' that uses 'B.breakSubstring' instead of 'B.findSubstrings'. Let me know if you want to keep 'splitByDelimiter', and I'll use that version.)

Redundant or unused? I don't see any changes other than removing the function.

The third patch removes 'TorDNSEL.System.Timeout'. There is a similar module in 'base'. […] There is another reason to remove 'TorDNSEL.System.Timeout': it's the only module that is not in the public domain.

I think this is because this module is actually based on an earlier version that what finally landed in base. Switching to that makes a lot of sense. -- Lunar <lunar@torproject.org>

Nikita Karetnikov:

Well, I agree. However, even obvious changes might be useful in the long term. The GNU Coding Standards, which I use as a guide, suggest the following: "Subsequent maintainers will often search for a function name to find all the change log entries that pertain to it..." [1] (For instance, cgit allows that.) It also helps when you're trying to fix bugs. Moreover, I often spot mistakes when I'm writing such messages.

I both agree, but I also try to keep in mind than the (at least most) GNU Coding Standards were thought in a time when we did not have version control solutions as good as git. Using `git blame` or `git log -S` or `git log -G` will actually work better than a file by file summary. If you throw in the `-M` option, it'll work accross renames, for examples.

Attached. Do you have any other comments? […] From 87ad0fe6f010b251aed042618dc82651294a477a Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov <nikita@karetnikov.org> Date: Thu, 25 Jul 2013 20:29:08 +0000 Subject: [PATCH 1/2] Reimport internal GHC functions from new locations.

---

Why not put what you wrote in your description email in there? This was a good explaination of why the change was actually needed! :)

"GHC was recently changed to not allow you to use newtypes in FFI imports unless the constructor of the newtype is in scope." [1]

[1] http://ghc.haskell.org/trac/ghc/ticket/5610

I am looking forward to the moment you'll have a patch set that builds. -- Lunar <lunar@torproject.org>

Nikita Karetnikov:

...

Using `git blame` or `git log -S` or `git log -G` will actually work better than a file by file summary. If you throw in the `-M` option, it'll work accross renames, for examples.

OK. But that information will be lost if we decide to change a VCS for some reason.

Well, we could avoid switching to a VCS that would loose such valuable information! :)

...

Why not put what you wrote in your description email in there? This was a good explaination of why the change was actually needed! :)

...

...

"GHC was recently changed to not allow you to use newtypes in FFI imports unless the constructor of the newtype is in scope." [1]

[1] http://ghc.haskell.org/trac/ghc/ticket/5610

OK. So let me summarize:

1. I shouldn't explicitly name changed functions or modules.

2. I can add a small description (like the above) if it's appropriate.

Did I forget anything?

Feel free to develop your own style and taste. Some readings: http://who-t.blogspot.de/2009/12/on-commit-messages.html http://robots.thoughtbot.com/post/48933156625/5-useful-tips-for-a-better-com... http://ablogaboutcode.com/2011/03/23/proper-git-commit-messages-and-an-elega... This is quite thorough: https://wiki.openstack.org/wiki/GitCommitMessages -- Lunar <lunar@torproject.org>

Yet another patch that (hopefully) fixes exceptions. If you apply it on top of the previous set [1], you should be able to build 24 out of 38 modules. [1] https://lists.torproject.org/pipermail/tor-dev/2013-August/005219.html