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.) [1] https://www.torproject.org/getinvolved/volunteer.html.en
On Thu, Jun 06, 2013 at 10:21:19AM +0400, Nikita Karetnikov wrote:
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.)
[1] https://www.torproject.org/getinvolved/volunteer.html.en
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/ --Roger
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, 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
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
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>
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?
Well, I'm interested. But I don't promise anything.
Even if my Haskell is rusty, I'd be happy to have a look at your changes, if you want/need some reviews. :)
Great. Can I contact you off-list? I don't want to overwhelm the list with basic questions.
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...
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
On Fri, Jun 21, 2013 at 4:10 PM, Nikita Karetnikov <nikita@karetnikov.org> wrote: Hello! I'll try to answer some of the easier stuff now, and put off the longer stuff till I have time after the weekend.
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?
The part I was most about was the discarding the extra bytes. Usually, when I see a function called "fooReadLine," I expect it not to throw away data away. That is, if I run "hGetLineN handle (B.pack "\n") 42" three times on a handle for input "foo\nbar\nbaz\n" , I would expect to get "foo", then "bar", then "baz". I don't know whether that's what hGetLine currently does though, but it's how I interpret its documentation. (As for whether to read up the the EOL first or whether to read up to the byte limit first: both can be problematic when the byte limit is high. But reading up to the EOL first is extra problematic, since it means that the program may keep reading forever if the user never sends an EOL.)
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?
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. best wishes, -- Nick
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/...
I apologize for the delay. Here are three patches. 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." [1] 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.) The third patch removes 'TorDNSEL.System.Timeout'. There is a similar module in 'base' (see [2], for instance). In TorDNSEL, this line (bracket (forkIO (threadDelay n >> throwDynTo pid ex)) wraps a 'Timeout' exception into 'Dynamic' [3] and raises the exception after 'n' microseconds. And this one handleJust (\e -> dynExceptions e >>= fromDynamic >>= guard . (ex ==)) catches dynamic exceptions and compares them with the 'Timeout' exception. I think there is no need to use dynamic exceptions at all. The latest version of 'System.Timeout' [4] is also similar but uses 'forkIOWithUnmask' [5]. It seems safer because it doesn't rely on 'MaskingState' [6] of a parent thread (also, see [7]). There is another reason to remove 'TorDNSEL.System.Timeout': it's the only module that is not in the public domain. If you apply the attached patches on top of the previous patchset [8], you should see some exception-related errors, which I haven't fixed yet. (Don't forget that the mentioned patches are preliminary and need additional testing.) [1] http://ghc.haskell.org/trac/ghc/ticket/5610 [2] http://hackage.haskell.org/packages/archive/base/4.1.0.0/doc/html/src/System... [3] http://hackage.haskell.org/packages/archive/base/4.1.0.0/doc/html/Control-Ol... [4] http://hackage.haskell.org/packages/archive/base/4.6.0.1/doc/html/src/System... [5] http://hackage.haskell.org/packages/archive/base/4.6.0.1/doc/html/src/GHC-Co... [6] http://hackage.haskell.org/packages/archive/base/4.6.0.1/doc/html/src/GHC-IO... [7] http://ofps.oreilly.com/titles/9781449335946/sec_cancellation.html [8] https://lists.torproject.org/pipermail/tor-dev/2013-July/005134.html
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>
Sorry for being slow to get to your patches.
No problem. I'm glad that someone is actually interested in that.
An overall comment: I am unconvinced about commit messages that details obvious changes for each impacted file.
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. Sometimes it's harder to write commit messages than code. So if you think that they are verbose, you're probably right.
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.
Unused, right.
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".
Attached. Do you have any other comments? [1] https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs
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]
I am looking forward to the moment you'll have a patch set that builds. -- Lunar <lunar@torproject.org>
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.
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]
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?
I am looking forward to the moment you'll have a patch set that builds.
It won't happen overnight. Right now I can only build eight modules. At least sixteen modules need adjustments. Not very impressive, yep.
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]
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>
I’m attaching three patches. For an explanation regarding the ‘FlexibleInstances’ extension see [1,2]. As always, I’ve only fixed critical things. Deprecation warnings can wait. Perhaps, I’ll combine all similar patches (like those that deal with exceptions) later. I’ve noticed that the author decided to use ‘Dynamic’-related functions to raise many (all?) exceptions. If you raise an exception of type ‘Dynamic’, you won’t get a meaningful message. Consider the following: {-# LANGUAGE DeriveDataTypeable #-} import qualified Control.Exception as E import Data.Dynamic (toDyn) import Data.Typeable (Typeable) -- | An exception related to links or monitors. data LinkException = NonexistentThread -- ^ deriving (Eq, Typeable) instance Show LinkException where show NonexistentThread = "Attempt to link to nonexistent thread" test1 = E.throw . toDyn $ NonexistentThread instance E.Exception LinkException where test2 = E.throw NonexistentThread In GHCi: *Main> test1 *** Exception: <<LinkException>> *Main> test2 *** Exception: Attempt to link to nonexistent thread Note that the first argument of ‘E.throw’ must be an instance of ‘E.Exception’: E.throw :: E.Exception e => e -> a ‘LinkException’ is not an instance of ‘E.Exception’ in ‘TorDNSEL.Control.Concurrent.Link.Internals’. It should be easy to change that. Later, I’d also like to inspect ‘withLinksDo’, ‘linkTogether’, and replace ‘$’ with ‘.’ in a couple of places. The previous set of patches is here [3]. [1] http://www.haskell.org/haskellwiki/List_instance [2] http://www.haskell.org/ghc/docs/6.8-latest/html/users_guide/type-class-exten... [3] https://lists.torproject.org/pipermail/tor-dev/2013-July/005157.html
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
These patches should allow you to build all modules (using GHC 7.6.3.) if you apply them on the previous set [1]. /But please don’t expect anything to work./ Now it’s time to actually test and reorganize the patches. Will it be sane to resend the previous 12 patches (approximately 1500 lines) to the list? Then I’m planning to review each patch since I’m not sure about some things (like the IncoherentInstances extension or the ‘hGetLine’ function). I’ll probably ask someone to help with that. Any comments? [1] https://lists.torproject.org/pipermail/tor-dev/2013-August/005246.html [2] http://www.haskell.org/ghc/docs/7.6.3/html/users_guide/type-class-extensions...
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-...
participants (5)
-
Andrew Lewman -
Lunar -
Nick Mathewson -
Nikita Karetnikov -
Roger Dingledine