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

Update boost to v1.70.0 to eliminate build warning #3951

Merged
merged 1 commit into from Apr 24, 2019

Conversation

@LarryRuane
Copy link
Contributor

commented Apr 12, 2019

Closes #3947.

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Changes are documented here: https://www.boost.org/users/history/version_1_70_0.html
I verified that the warning no longer appears when building (zcutil/build.sh) but I didn't try running any of the tests.

@defuse

defuse approved these changes Apr 15, 2019

Copy link
Contributor

left a comment

ACK. I checked the hash, looked over the changelog for fixed security issues, and ran the tests.

@bitcartel bitcartel self-requested a review Apr 16, 2019

@bitcartel
Copy link
Contributor

left a comment

utACK. Verified hash against download announced here https://www.boost.org/users/history/version_1_70_0.html

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Has a Gitian build been run against this branch/commit to verify updating the dependency doesn't break Gitian?

@str4d

str4d approved these changes Apr 16, 2019

Copy link
Contributor

left a comment

utACK. I verified that the hash matches the dependency as downloaded by me.

@mms710 mms710 added this to Needs Prioritization in Arborist Team via automation Apr 16, 2019

@mms710 mms710 moved this from Needs Prioritization to Bugs/Security Issues/TechDebt Backlog in Arborist Team Apr 16, 2019

@mms710 mms710 moved this from Bugs/Security Issues/TechDebt Backlog to PRs That Need Review + Their Associated Issues in Arborist Team Apr 16, 2019

@mms710 mms710 moved this from PRs That Need Review + Their Associated Issues to In Progress in Arborist Team Apr 16, 2019

@mms710 mms710 moved this from In Progress to Merge Queue in Arborist Team Apr 16, 2019

@daira

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

utACK. I did not check the hash. I did read the changelog and found the following potentially-relevant-to-us fixed security issues in the Filesystem component:

  • Fixed a few instances of dereferencing std::string::end() in path implementation.
  • Fixed possible use of uninitialized data in directory iterator increment operation on Linux.
@daira

daira approved these changes Apr 16, 2019

Copy link
Contributor

left a comment

(review in comment above)

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Charlie's helping me set up my system to do Gitian builds. You may want to wait for that before r+. I'll post here when I've finished testing that.

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Well, it seems I can't do a Gitian build because that requires VirtualBox, but I run my Linux in VB and nested virtualization doesn't seem to be supported. But @charlieok said he'd try tomorrow. (Thanks, Charlie!)

@garethtdavies

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I tried a Gitian build of this and it failed with

In file included from bitcoind.cpp:9:0:
main.h:29:26: fatal error: addressindex.h: No such file or directory
 #include "addressindex.h"
                          ^
compilation terminated.
Makefile:5689: recipe for target 'zcashd-bitcoind.o' failed
make[2]: *** [zcashd-bitcoind.o] Error 1
make[2]: Leaving directory '/home/debian/build/zcash/distsrc-x86_64-unknown-linux-gnu/src'
Makefile:5737: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/debian/build/zcash/distsrc-x86_64-unknown-linux-gnu/src'
Makefile:615: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1

To check I tried the current Zcash master and this also failed with the same. Checking through commits since 2.0.4 looks like this one is the issue e73326e

I confirmed this by running the previous commit 926fd0c and that built fine and indeed e73326e fails.

So, I guess this can't be confirmed to build via Gitian until that issue is resolved?

@garethtdavies

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I cherrypicked this commit and applied on top of v2.0.4 and that does indeed build via Gitian.

@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

gitian_hashes.txt

I did a gitian build of the latest commit on this PR: LarryRuane@dfc13c5

The generated file of associated hashes is attached, on the assumption that the gitian concern was about the newer version of boost breaking reproducibility, in case anyone else would like to compare their result with mine.

@garethtdavies

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@charlieok did that actually complete or did it fail as looking at those hashes the out_manifest is empty? I just destroyed my VM and recreated and still get the following in gitian-builder/var/build.log when building this commit (but it does create the hashes).

main.h:29:26: fatal error: addressindex.h: No such file or directory
 #include "addressindex.h"
@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@garethtdavies Thanks for catching that. I just quickly ran the build and copied the result here without inspecting it. I destroyed the VM afterward but I'll try running it again.

@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Yes, it looks like the gitian build is failing. Attaching the contents of /home/vagrant/gitian-builder/var/build.log in my VM:
build.log

@garethtdavies

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

That looks like a different issue and possibly related to the latest commit to zcash-gitian as curl now isn't available? zcash/zcash-gitian@367b6e9

@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@garethtdavies I did wonder about that but I verified that curl was installed in the VM (as well as all the other packages in the two lists in that update):

vagrant@zcash-build:~$ dpkg --status curl
Package: curl
Status: install ok installed
Priority: optional
Section: web
Installed-Size: 346
Maintainer: Alessandro Ghedini <ghedo@debian.org>
Architecture: amd64
Multi-Arch: foreign
Version: 7.52.1-5+deb9u9
Depends: libc6 (>= 2.17), libcurl3 (= 7.52.1-5+deb9u9), zlib1g (>= 1:1.1.4)
Description: command line tool for transferring data with URL syntax
 curl is a command line tool for transferring data with URL syntax, supporting
 DICT, FILE, FTP, FTPS, GOPHER, HTTP, HTTPS, IMAP, IMAPS, LDAP, LDAPS, POP3,
 POP3S, RTMP, RTSP, SCP, SFTP, SMTP, SMTPS, TELNET and TFTP.
 .
 curl supports SSL certificates, HTTP POST, HTTP PUT, FTP uploading, HTTP form
 based upload, proxies, cookies, user+password authentication (Basic, Digest,
 NTLM, Negotiate, kerberos...), file transfer resume, proxy tunneling and a
 busload of other useful tricks.
Homepage: http://curl.haxx.se

vagrant@zcash-build:~$ type curl
curl is /usr/bin/curl
@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Just tried rebuilding using commit zcash/zcash-gitian@0abc644 to verify that my recent update to zcash-gitian did not cause the error I posted above. It did not -- it failed in the same way.

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Thanks, @garethtdavies and @charlieok, I don't understand what the problem with e73326e is; that commit did add a new source file, src/addressindex.h, but it's clearly part of that commit (and of course the normal build succeeds). I'm not sure what to do now, maybe create a new ticket that says Gitian build doesn't succeed with current zcash master?

@charlieok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Apologies for the noise above. My posts above were in response to a request from @LarryRuane to run a gitian build on this branch, and I did not see until after my initial post that @garethtdavies had already found an issue and apparently traced it to its source.

@mms710 mms710 moved this from Merge Queue to In Review in Arborist Team Apr 22, 2019

@mms710

This comment has been minimized.

Copy link

commented Apr 22, 2019

We're blocking this PR on getting #3966 just so we can make sure we have a passing build before we merge things.

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team Apr 23, 2019

@daira

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

📌 Commit dfc13c5 has been approved by daira

@LarryRuane LarryRuane force-pushed the LarryRuane:3947-integer-log2-warn branch from dfc13c5 to c17d828 Apr 23, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I just rebased from zcash master to pick up the Gitian build fix #3966.

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

📌 Commit c17d828 has been approved by ebfull

zkbot added a commit that referenced this pull request Apr 23, 2019

Auto merge of #3951 - LarryRuane:3947-integer-log2-warn, r=ebfull
Update boost to v1.70.0 to eliminate build warning

Closes #3947.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

⌛️ Testing commit c17d828 with merge fd5836e...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Transient failure, timed-out.

=== Running testscript key_import_export.py ===
command timed out: 1200 seconds without output running ['./qa/zcash/full_test_suite.py', 'rpc'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=4473.756010

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

⌛️ Testing commit c17d828 with merge 4e3f8b4...

zkbot added a commit that referenced this pull request Apr 24, 2019

Auto merge of #3951 - LarryRuane:3947-integer-log2-warn, r=ebfull
Update boost to v1.70.0 to eliminate build warning

Closes #3947.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Yet another transient failure.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

⌛️ Testing commit c17d828 with merge 8a95ba2...

zkbot added a commit that referenced this pull request Apr 24, 2019

Auto merge of #3951 - LarryRuane:3947-integer-log2-warn, r=ebfull
Update boost to v1.70.0 to eliminate build warning

Closes #3947.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing 8a95ba2 to master...

@zkbot zkbot merged commit c17d828 into zcash:master Apr 24, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Apr 24, 2019

@mms710 mms710 added this to the v2.0.5 milestone Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.