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

feat: add fauxrep2 to api #124

Merged
merged 9 commits into from
Jul 29, 2020
Merged

feat: add fauxrep2 to api #124

merged 9 commits into from
Jul 29, 2020

Conversation

cryptonemo
Copy link
Contributor

feat: update dependencies (including proofs)

@cryptonemo cryptonemo requested a review from magik6k July 28, 2020 18:37
@cryptonemo
Copy link
Contributor Author

cryptonemo commented Jul 28, 2020

This PR is incomplete in that the tests cannot be run because make cgo-gen is failing. That step needs to be run, and then committed on top of this. @magik6k is this something you can do? Apparently, even on master, make cgo-gen fails for me.

@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

That sounded like an issue I'm good at figuring out. Half an hour later it runs for me (though I have too little clue about Go/CGo, so I can't really verify if it does the right thing). Works on my Debian testing:

cd /tmp
git clone git@github.com:xlab/c-for-go.git
cd c-for-go
git revert e66ad96c6dd2729b41eae75eca1367942fd11492 --no-edit
go build .

# Go to your filecoin-ffi checkout and run:
/tmp/c-for-go/c-for-go --ccincl --nostamp filcrypto.yml

@Kubuxu
Copy link

Kubuxu commented Jul 29, 2020

I still cannot use it as on my system rerunning it results with every uint64 being replace with uint. Maybe someone should try on OSX.

@cryptonemo
Copy link
Contributor Author

cryptonemo commented Jul 29, 2020

@vmx This also doesn't work for me (same error I was seeing before). I'm also on debian testing.

@cryptonemo
Copy link
Contributor Author

cryptonemo commented Jul 29, 2020

@vmx Scratch that! So the README Makefile says to use --ccdefs in addition to the args you mentioned, so I was using that pasted command. As you posted, that flag is removed and it works fine for me now. Let me do some testing and update this PR -- thanks!

@cryptonemo
Copy link
Contributor Author

@vmx Ok, confirmed that I'm in the same boat now as @Kubuxu -- uint64 replacements causing errors.

@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

@cryptonemo Though I'm seeing the same uint64_t -> uint issue as @Kubuxu has.

@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

I propose letting someone on OSX run that (without modifications first) and see if that works. If it doesn't work, fixing it couldn't be to hard (esp. if you know Go ;)

@cryptonemo
Copy link
Contributor Author

cryptonemo commented Jul 29, 2020

@vmx Ok. Does anyone on this PR/thread have OS X to run this on though? It's probably pretty important that someone here can get this going w/o too much issue in the future.

@cryptonemo
Copy link
Contributor Author

I looked at this a bit and we can actually use the standard c-for-go version to process the filecrypto.yml file if the 'Defines:' and __has_include_next(x): 1 lines are removed. Same uint64/uint error after though. Not sure if this is helpful or not, but I found it interesting.

@Kubuxu
Copy link

Kubuxu commented Jul 29, 2020

So the difference seems to be that osx is using long long for some type and Linux is using long.

@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

So the difference seems to be that osx is using long long for some type and Linux is using long.

Though it transforms from our own filecoin.h the unit64_ts into uints which is really strange.

@Kubuxu
Copy link

Kubuxu commented Jul 29, 2020

Maybe linux declares uint64_t as long.

@Kubuxu
Copy link

Kubuxu commented Jul 29, 2020

For me uint64_t is declared as: unsigned long int

@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

Maybe linux declares uint64_t as long.

I don't know if that's really the core file of GCC where things are defined. but here https://github.com/gcc-mirror/gcc/blob/f6fe3bbf9f6c0b7249933e19b94560b6b26bf269/libphobos/libdruntime/core/stdc/stdint.d I can see:

version (Darwin)
…
    alias uint64_t = cpp_ulonglong; ///
…
else version (Posix)
…
    alias uint64_t = ulong;  ///

That sucks.

Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
Jakub Sztandera and others added 7 commits July 29, 2020 18:20
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
,
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
@Kubuxu Kubuxu merged commit f5f0f80 into master Jul 29, 2020
@Kubuxu Kubuxu deleted the fauxrep2 branch July 29, 2020 21:45
@vmx
Copy link
Contributor

vmx commented Jul 29, 2020

@Kubuxu Do I understand it correctly that your c-for-go fork just applied xlab/c-for-go#91? So if we can get that one merged upstream, we could switch back to that?

@Kubuxu
Copy link

Kubuxu commented Jul 29, 2020

Yeah, it is just that.

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.

None yet

3 participants