Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add EdDSA support + static libsodium #67

Merged
merged 5 commits into from
Dec 12, 2016
Merged

Re-add EdDSA support + static libsodium #67

merged 5 commits into from
Dec 12, 2016

Conversation

olorin
Copy link
Contributor

@olorin olorin commented Dec 12, 2016

The only functional change in this PR (measured from before Ed25519 support was removed) is a libsodium version upgrade.

! @novemberkilo @erikd-ambiata @thumphries

Release 1.0.11, commit 2f4f718cd94adab547c210a78050762cf667dfca.
@olorin
Copy link
Contributor Author

olorin commented Dec 12, 2016

configure.ac:1: error: Autoconf version 2.65 or higher is required

😡

I will work around this.

@erikd-ambiata
Copy link

erikd-ambiata commented Dec 12, 2016

configure.ac:1: error: Autoconf version 2.65 or higher is required

I will work around this.

2.65 is ancient. Good old RHEL!

#include "constants.h"

size_t tinfoil_sodium_pubkey_len() {
return crypto_sign_PUBLICKEYBYTES;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need void in the (empty) parameter list when doing C.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep thanks, I thought I'd fixed that here but must have missed a few functions.

#include <sodium.h>

size_t tinfoil_sodium_pubkey_len();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -154,7 +163,7 @@ benchmark bench

main-is: bench.hs

ghc-options: -Wall -threaded -O2
ghc-options: -Wall -threaded -O2 -pgml ./bin/salted-gcc
Copy link

@erikd-ambiata erikd-ambiata Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want -Wextra there as well just to see the extra warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Wextra for the C code right? You're right, but shouldn't it go in cc-opts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, misread that, but -Wextra for the C code, always :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, added, annoyed I didn't have it on from the start.

{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE GADTs #-}
{-# OPTIONS_GHC -fno-warn-missing-signatures #-}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you actually have signatures for everything, maybe worth dropping -fno-warn-missing-signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@erikd-ambiata
Copy link

Totally gnored the libsodium code. Making sure empty parameter lists for C functions are specified as (void) is important (may be picked up my -Wextra).

👍 in C parameter lists and -Wextra.

@erikd-ambiata
Copy link

On of the nasty little differences between C and C++ : http://stackoverflow.com/questions/416345/is-fvoid-deprecated-in-modern-c-and-c

@olorin
Copy link
Contributor Author

olorin commented Dec 12, 2016

On of the nasty little differences between C and C++

Yeah, I remember that one, you explained it to me a while back (I think in another project) - I think I did the initial version of the bindings before that and forgot to come back and fix it.

@thumphries
Copy link
Contributor

Setup.hs looks good to me

No opinion on the giant subtree

@thumphries thumphries removed their assignment Dec 12, 2016
@olorin olorin merged commit ac2d5e7 into master Dec 12, 2016
@olorin olorin deleted the topic/v1 branch December 12, 2016 23:12
@olorin olorin mentioned this pull request Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants