On Sat, Jun 8, 2013 at 11:58 AM, Nikita Karetnikov nikita@karetnikov.org wrote:

...

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?

There's a tradeoff that you need to consider with licensing decisions. If you pick a permissive license, then there's a risk that somebody might do a proprietary fork. This seems to be a higher risk with some kinds of software than others: I don't personally think that there's much likelihood that somebody would try to make non-free changes to TorDNSEL. (After all, nobody has tried to make non-free changes to the current version in the past N years.)

If you pick a restrictive copyleft like one of the GPL licenses, on the other hand, then nobody can, but other free software projects that have chosen other licenses won't be able to use your code. For instance, if you pick GPLv3, then no project that links OpenSSL can use your software. If you write some interesting bit of Haskell code that a BSD-licensed Haskell project would like to use, they well probably sigh and choose not to use it, even if they would really like to.

So when you pick a copyleft-style license, you prevent non-free forks at the expense of also preventing a large number of free uses of your software.

Personally, I would leave it public domain (probably via a CC0-style statement), or put it under a permissive license (most of Tor is under 3-clause BSD). My reasoning is more or less that I don't see a big commercial market here that would *want* to rip off TorDNSEL. But this is one of those issues where reasonable people will disagree and in my opinion it's fine to let the programmers decide.

I'm not qualified to answer your legal question about the force of the public domain dedication on the code as it stands.

best wishes, -- Nick

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.

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.)

I tried with GHC 6.12.1 as well:

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(..)'

GHC 6.12.1 uses a different version of 'base' (see [1]). That's why the above errors appeared.

After that I also tried to build with GHC 7.6.3 and got the same errors. What version of GHC do you use? What about versions of libraries? Note that 'tordnsel.cabal' says that the package should work with GHC 6.6, GHC 6.8, GHC 6.10, and GHC 6.12.

I'm attaching two /preliminary/ patches:

* 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'.

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

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 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.

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

What do you think?

[1] http://www.haskell.org/ghc/docs/6.12.1/html/users_guide/release-6-12-1.html [2] http://stackoverflow.com/a/5588790 [3] http://hackage.haskell.org/packages/archive/binary/0.4.5/doc/html/src/Data-B... [4] http://hackage.haskell.org/packages/archive/binary/0.7.1.0/doc/html/src/Data... [5] http://hackage.haskell.org/packages/archive/binary/0.6.0.0/doc/html/src/Data...

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.

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.

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

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

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-...