Skip to content

Commit

Permalink
Add libzerocash to the depends system. Very crude. #263
Browse files Browse the repository at this point in the history
  • Loading branch information
@defuse
defuse committed Oct 7, 2015
1 parent 7ad6f5c commit dd5c000
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 1 deletion.
32 changes: 32 additions & 0 deletions depends/packages/libzerocash.mk
View file
@@ -0,0 +1,32 @@
package=libzerocash
# FIXME: what is the correct version number?
$(package)_version=0.1
$(package)_download_path=https://github.com/Electric-Coin-Company/$(package)/archive/
$(package)_file_name=$(package)-$($(package)_git_commit).tar.gz
$(package)_download_file=$($(package)_git_commit).tar.gz
$(package)_sha256_hash=0f157cc145844b21dbd601f2da6bdba887b4204699eee347b48918ee726b3cb4
$(package)_git_commit=89aded7b2a59d5d589e053cd4ebc2b06b29aa5cf


# FIXME: are there any more dependencies I forgot to list (probably!)?
$(package)_dependencies=libsnark crypto++
$(package)_patches=1_install_target.patch

define $(package)_preprocess_cmds
patch -p1 < $($(package)_patch_dir)/1_install_target.patch
endef

# FIXME: How do we know libsnark (etc.) already exists there?
# What's the proper way for one depends-package to depend on another
# depends-package?

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

I thought that $(package)_dependencies above would ensure those dependencies were somehow available during the build of the current $(package).
$(package)_cppflags += -I. -I$(host_prefix)/include -I$(host_prefix)/include/libsnark -DCURVE_ALT_BN128 -DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -std=c++11 -pipe -O2 -O0 -g -Wstack-protector -fstack-protector-all -fPIE -fvisibility=hidden

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

I think that other ./depends packages don't need custom -I paths... if this works, it's acceptable, but it would be nice if libsnark didn't require a non-standard include path, and then ./depends did the same thing for libsnark / libzerocash as other packages.

# FIXME: The CXXFLAGS given here overrules the one in the libzerocash
# Makefile... Maybe we should only prefix/append?

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

When I originally "transplanted" libzerocash into this repository, added a PREFIX_* expansion at the beginning of the Make variables, as in:

CXXFLAGS = $(CXXFLAGS_PREFIX) ...<rest of the flags defined above>

This was just another way to hack around the same issue. One reason I prefer this approach is that we can then distinguish which flags are "baked in" to libzerocash and which are site-specific customizations, like -I. If -I is the only reason we need to customize CXXFLAGS in the first place, then we should fix upstream's non-standard use of header paths.

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

Filed #294 to deal with this after this PR/ticket.
define $(package)_build_cmds
$(MAKE) all DEPINST=$(host_prefix) CXXFLAGS="$($(package)_cppflags)"
endef

define $(package)_stage_cmds
$(MAKE) install STATIC=1 PREFIX=$($(package)_staging_dir)$(host_prefix)
endef
2 changes: 1 addition & 1 deletion depends/packages/packages.mk
View file
@@ -1,4 +1,4 @@
zerocash_packages := libsnark crypto++ libgmp xbyak ate-pairing
zerocash_packages := libsnark crypto++ libgmp xbyak ate-pairing libzerocash
packages:=boost openssl $(zerocash_packages)
native_packages := native_ccache native_comparisontool

Expand Down
39 changes: 39 additions & 0 deletions depends/patches/libzerocash/1_install_target.patch
View file
@@ -0,0 +1,39 @@
diff --git a/Makefile b/Makefile

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

It would be nice if this patch explained it's purpose.

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

Ah, @defuse tells me he filed #293 to replace these patches with actual commits to our libzerocash fork.
index 7d82a06..6074391 100644
--- a/Makefile
+++ b/Makefile
@@ -36,6 +36,7 @@ EXECUTABLES= \

OBJS=$(patsubst %.cpp,%.o,$(SRCS))

+
ifeq ($(MINDEPS),1)
CXXFLAGS += -DMINDEPS
else
@@ -110,7 +111,7 @@ banktest_library: %: bankTest.o $(OBJS)
merkletest_library: %: merkleTest.o $(OBJS)

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

Was merkletest_library not build in upstream?

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

Oh, I was misreading because this is a commit delta of a patch file (a patch of a patch)... -and this line is just unchanged context.
$(CXX) -o $@ $^ $(CXXFLAGS) $(LDFLAGS) $(LDLIBS) -lzerocash

-.PHONY: clean
+.PHONY: clean install

clean:
$(RM) \
@@ -120,3 +121,17 @@ clean:
${patsubst %.cpp,%.d,${SRCS}} \
libzerocash.a \
tests/test_library
+
+# FIXME: Copied crudely from libsnark...

This comment has been minimized.

Sign in to view

Copy link
@defuse

defuse Oct 7, 2015

Author Contributor

To the reviewer: Look at this carefully. Is it good enough?
+HEADERS_SRC=$(shell find . -name '*.hpp' -o -name '*.tcc' -o -name '*.h')
+HEADERS_DEST=$(patsubst %,$(PREFIX)/include/libzerocash/%,$(HEADERS_SRC))

This comment has been minimized.

Sign in to view

Copy link
@nathan-at-least

nathan-at-least Oct 7, 2015

Contributor

Note, this means there are ${SYSTEM_INCLUDE_DIR}/libzerocash/libzerocash and ${SYSTEM_INCLUDE_DIR}/libzerocash/zerocash_pour_ppzksnark directories. I prefer having only a single top-level include dir per pacakage, so I like this, but the upstream authors may have intended both directories to live immediately in the system include dir.
+
+$(HEADERS_DEST): $(PREFIX)/include/libzerocash/%: %
+ mkdir -p $(shell dirname $@)
+ cp $< $@
+
+# TODO: install the test executables into the bin/ directory.
+install: all $(HEADERS_DEST)
+ mkdir -p $(PREFIX)/lib
+ install -m 0755 libzerocash.a $(PREFIX)/lib/
+

1 comment on commit dd5c000

@nathan-at-least
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, with the caveat of linked tickets for future work.

Please sign in to comment.