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.

Sign up for GitHub

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

Jump to bottom

Disable building Proton JavaScript bindings and other non-essential stuff #2280

Merged

zkbot merged 1 commit into zcash:master from str4d:2279-disable-proton-js-binding

May 24, 2017

Contributor

str4d commented Apr 19, 2017

Closes #2279.

str4d added C-bug A-dependencies labels

str4d added this to the 1.0.9 milestone

str4d requested a review from bitcartel

April 19, 2017 01:39

str4d mentioned this pull request

Proton dependency fails to build #2279

Closed

daira previously approved these changes

View reviewed changes

Contributor

daira left a comment

utACK.

Does not block, but why do DEFAULT_JAVASCRIPT and DEFAULT_GO need to be set to OFF both in CMakeLists.txt and on the cmake command line?

daira mentioned this pull request

build: zcutils/build.sh fails on non-debian based 64-bit Linux distros #2294

Closed

Contributor

bitcartel commented Apr 27, 2017

@daira @str4d I've pushed a commit to this PR for review. Cleans up building. See commit message; no longer builds docs, examples, language bindings (except for cpp), also does not try and find packages which we know we don't need.

Contributor

bitcartel commented Apr 27, 2017

Contributor

daira commented Apr 27, 2017

The sed commands look like they could be better expressed as a patch; that would be easier to review.

Contributor

zkbot commented Apr 27, 2017

💥 Test timed out

bitcartel force-pushed the 2279-disable-proton-js-binding branch from ef54438 to 28174a7 Compare

April 28, 2017 01:21

Contributor

bitcartel commented Apr 28, 2017

Contributor

ageis commented Apr 28, 2017

@zkbot retry

daira reviewed

View reviewed changes

depends/patches/proton/minimal-build.patch

    
               # Set the default SSL/TLS implementation

              -find_package(OpenSSL)

              +#find_package(OpenSSL)

               find_package(PythonInterp REQUIRED)

Contributor

daira Apr 28, 2017

Should this be disabled?

Contributor

bitcartel Apr 28, 2017

No, if disabled, build fails. There are generated source files which require Python.

depends/patches/proton/minimal-build.patch

    
               # Check for valgrind here so tests under proton-c/ and examples/ can use it.

              -find_program(VALGRIND_EXE valgrind DOC "Location of the valgrind program")

              +#find_program(VALGRIND_EXE valgrind DOC "Location of the valgrind program")

               mark_as_advanced (VALGRIND_EXE)

Contributor

daira Apr 28, 2017

Comment this out as well.

Contributor

bitcartel Apr 28, 2017

I think you mean that you want "#mark_as_advanced" ? From cmake docs, 'mark_as_advanced' doesn't do anything with regards to the build, it will hide the variable from the CMake GUI. I didn't comment it out as I wanted the patch to be as small as possible.

depends/patches/proton/minimal-build.patch

    
              -find_package(SWIG)

              +#find_package(SWIG)

               # FindSwig.cmake "forgets" make its outputs advanced like a good citizen

               mark_as_advanced(SWIG_DIR SWIG_EXECUTABLE SWIG_VERSION)

Contributor

daira Apr 28, 2017

Comment this out as well.

Contributor

bitcartel Apr 28, 2017

See above about mark_as_advanced.

depends/patches/proton/minimal-build.patch

    
              +#find_library(CYRUS_SASL_LIBRARY sasl2)

              +#find_path(CYRUS_SASL_INCLUDE_DIR sasl/sasl.h PATH_SUFFIXES include)

              +#find_package_handle_standard_args(CyrusSASL DEFAULT_MSG CYRUS_SASL_LIBRARY CYRUS_SASL_INCLUDE_DIR)

               mark_as_advanced(CYRUS_SASL_LIBRARY CYRUS_SASL_INCLUDE_DIR)

Contributor

daira Apr 28, 2017

Comment this out as well.

Contributor

bitcartel Apr 28, 2017

Ditto

depends/patches/proton/minimal-build.patch

    
               # Find saslpasswd2 executable to generate test config

              -find_program(SASLPASSWD_EXE saslpasswd2 DOC "Program used to make SASL user db for testing")

              +#find_program(SASLPASSWD_EXE saslpasswd2 DOC "Program used to make SASL user db for testing")

               mark_as_advanced(SASLPASSWD_EXE)

Contributor

daira Apr 28, 2017

Comment this out as well.

Contributor

bitcartel Apr 28, 2017

Ditto

depends/patches/proton/minimal-build.patch

    
               # Prerequisites for Go

              -find_program(GO_EXE go)

              +#find_program(GO_EXE go)

               mark_as_advanced(GO_EXE)

Contributor

daira Apr 28, 2017

Comment this out as well.

Contributor

bitcartel Apr 28, 2017

Ditto

depends/patches/proton/minimal-build.patch Outdated

    
              From 03f5fc0826115edbfca468261b70c0daf627f488 Mon Sep 17 00:00:00 2001

              From: Simon <simon@bitcartel.com>

              Date: Thu, 27 Apr 2017 17:15:59 -0700

              Subject: [PATCH] Only build minimal dependencies needed for cpp bindings.

Contributor

daira Apr 28, 2017

Add that this enables C++11 and builds a static library.

Contributor

daira Apr 28, 2017

... or separate that into another patch maybe. (Does not block.)

Contributor

bitcartel Apr 28, 2017

I'll change the subject (and commit message) in a bit.

Contributor

bitcartel Apr 28, 2017

Fixed.

daira dismissed their stale review

April 28, 2017 13:24

need responses to comments on 28174a7

Contributor

zkbot commented Apr 28, 2017

⌛ Trying commit 28174a7 with merge de11d7b...

bitcartel changed the title ~~Disable building Proton JavaScript bindings~~ Disable building Proton JavaScript bindings and other non-essential stuff


          Patch to build Proton with minimal dependencies.

b9f6e40

Closes zcash#2279.  Configures CMake to enable C++11, build static libaries
and only build cpp bindings with minimal dependencies. Documentation,
examples, tests and other language bindings are no longer built.
CMake will no longer try to find commands and packages which are not
required for building the target.

bitcartel force-pushed the 2279-disable-proton-js-binding branch from 28174a7 to b9f6e40 Compare

April 28, 2017 19:09

bitcartel reviewed

View reviewed changes

Contributor

bitcartel left a comment

New PR pushed.

Contributor

bitcartel commented Apr 28, 2017

Checked CI server and buildbot tests passed.
http://ci.z.cash:8010/#/builders/14/builds/1
Note, this is a temporary link before CI servers get reset and rebuilt (at which point we can delete some of the zkbot related messages on this ticket)

Contributor

ageis commented Apr 28, 2017

retrying due to previous failure of Buildbot to tell Homu about the final build status and config update to fix the issue @zkbot retry

zkbot added a commit that referenced this pull request


          Auto merge of #2280 - str4d:2279-disable-proton-js-binding, r=<try>

980aa21

Disable building Proton JavaScript bindings and other non-essential stuff

Closes #2279.

Contributor

zkbot commented Apr 28, 2017

⌛ Trying commit b9f6e40 with merge 980aa21...

Contributor

ageis commented Apr 28, 2017

Contributor

zkbot commented Apr 29, 2017

💥 Test timed out

daira approved these changes

View reviewed changes

Contributor

daira left a comment

utACK

zkbot added a commit that referenced this pull request


          Auto merge of #2280 - str4d:2279-disable-proton-js-binding, r=<try>

9e351f2

Disable building Proton JavaScript bindings and other non-essential stuff

Closes #2279.

zkbot added a commit that referenced this pull request


          Auto merge of #2280 - str4d:2279-disable-proton-js-binding, r=str4d

79cf522

Disable building Proton JavaScript bindings and other non-essential stuff

Closes #2279.

Contributor Author

str4d commented May 10, 2017

Future readers: the various merges above were created while I was testing the CI. I have deleted the corresponding commands and responses to clean up the PR history.

Contributor

nathan-at-least commented May 10, 2017

I'm kicking this out of 1.0.9 in favor of #2362. Let's follow up on this after 1.0.9.

nathan-at-least removed this from the 1.0.9 milestone

Contributor

bitcartel commented May 11, 2017

@nathan-at-least This should be included in 1.0.9 because it fixes potential build problems for people who do want to build proton. Otherwise some people will opt in to build and be confused as to why it doesn't.

bitcartel mentioned this pull request

Disable building Proton by default #2362

Merged

Contributor

nathan-at-least commented May 11, 2017

If I understand correctly, this branch passes on our CI builder that doesn't have the other languages installed, so our CI system does not test the build fixes that this patch introduces. We need to set up a CI builder specifically to test those changes, and I don't believe we have time to do so today.

nathan-at-least mentioned this pull request

Set up a new builder for regression tests against Proton build issue #2279 #2365

Closed

Contributor

nathan-at-least commented May 11, 2017

I filed #2365 to follow up on the CI need here.

nathan-at-least added this to the 1.0.9 milestone

Contributor

nathan-at-least commented May 11, 2017

I moved #2365 and this PR back into 1.0.9 low on the page, indicating lower priority than the other tickets in 1.0.9.

nathan-at-least added this to Work Queue in Development Infrastructure

Contributor

ageis commented May 18, 2017

We should make a list of stuff to build without this patch:

openssl
go
sasl2
swig
ruby
doxygen

Contributor

bitcartel commented May 19, 2017

@ageis valgrind, emscripten, cyrus-sasl, perl, python

daira modified the milestones: 1.0.10, 1.0.9

Contributor

nathan-at-least commented May 24, 2017

Just completed #2365, so:

Contributor

zkbot commented May 24, 2017

📌 Commit b9f6e40 has been approved by nathan-at-least

Contributor

zkbot commented May 24, 2017

⌛ Testing commit b9f6e40 with merge 25d47f4...

zkbot added a commit that referenced this pull request


          Auto merge of #2280 - str4d:2279-disable-proton-js-binding, r=nathan-…

25d47f4

…at-least

Disable building Proton JavaScript bindings and other non-essential stuff

Closes #2279.

bitcartel modified the milestones: 1.0.9, 1.0.10

Contributor

zkbot commented May 24, 2017

☀️ Test successful - pr-merge
Approved by: nathan-at-least
Pushing 25d47f4 to master...

zkbot merged commit b9f6e40 into zcash:master

str4d moved this from Work Queue to Complete in Development Infrastructure

This was referenced Jan 30, 2019

Review dependencies for security/reliability updates #3786

Open

Dependency Updates #3809

Merged

str4d deleted the 2279-disable-proton-js-binding branch

January 27, 2023 17:01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment