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

depends: Explicitly download and vendor Rust dependencies #3096

Merged
merged 1 commit into from Mar 28, 2018

Conversation

Projects
None yet
5 participants
@str4d
Copy link
Contributor

commented Mar 17, 2018

Closes #2231.

@str4d str4d added this to the v1.1.0 milestone Mar 17, 2018

@str4d str4d requested review from ebfull, arcalinea and mdr0id Mar 17, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

Alternative to #3094.

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

Add --frozen to cargo build

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

⌛️ Trying commit a846c96 with merge 6a8ca6e...

zkbot added a commit that referenced this pull request Mar 20, 2018

Auto merge of #3096 - str4d:2231-depends-vendor-rust-crates, r=<try>
depends: Explicitly download and vendor Rust dependencies

Closes #2231.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

💔 Test failed - pr-try

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Failed due to an odd ccache error during the Debian 9 build that I haven't seen before.


define configure_crate_registry
mkdir .cargo && \
echo "[source.vendored-sources]\ndirectory = \"$(host_prefix)/$(CRATE_REGISTRY)\"\n\n[source.crates-io]\nreplace-with = \"vendored-sources\"" > .cargo/config

This comment has been minimized.

Copy link
@ebfull

ebfull Mar 20, 2018

Contributor

These newlines aren't going to work right depending on the platform. (In the homu try, you can see the mac build failed because these weren't escaped.)

This comment has been minimized.

Copy link
@ebfull

ebfull Mar 20, 2018

Contributor

I think it would be better to have a template file that gets sed'd, allowing us to easily change other configurations or target features.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

These newlines aren't going to work right depending on the platform. (In the homu try, you can see the mac build failed because these weren't escaped.)

The MacOS build actually failed because of issues in the command that generates the .cargo-checksum.json file. It seems that the intermediate sed that converts sha256sum output H(A) A to "A":"H(A)" is not working. I'll dig into that.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

@ebfull addressed your comments.

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2018

⌛️ Trying commit 3913c29 with merge d13b405...

zkbot added a commit that referenced this pull request Mar 21, 2018

Auto merge of #3096 - str4d:2231-depends-vendor-rust-crates, r=<try>
depends: Explicitly download and vendor Rust dependencies

Closes #2231.
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

I can squash the last two commits into the first one if desired.

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2018

☀️ Test successful - pr-try
State: approved= try=True

@@ -30,6 +30,18 @@ define fetch_file
rm -rf $$($(1)_download_dir) ))
endef

define generate_crate_checksum
rm .gitignore && \
echo "{\"files\":{`find . -type f | grep -v "$($(1)_file_name)" | grep -v .stamp_ | sed 's|^./||' | sort | xargs $(build_SHA256SUM) | awk -v OFS='":"' '{print $$$$2, $$$$1}' | sed 's|^|"|' | sed 's|$|"|' | tr '\n' ',' | sed 's|,$$$$||'`},\"package\":\"$($(1)_sha256_hash)\"}" > .cargo-checksum.json

This comment has been minimized.

Copy link
@daira

daira Mar 21, 2018

Contributor

Oww my head. Sorry, the interaction of quoting levels and of shell vs make expansion here is just impossible to review. NACK until it is simplified somehow (maybe an intermediate variable for the result of the backticked expression?)

This comment has been minimized.

Copy link
@daira

daira Mar 21, 2018

Contributor

It doesn't help that it's all on one line.

This comment has been minimized.

Copy link
@daira

daira Mar 21, 2018

Contributor

Maybe a separate shell script? That would allow the steps to be split up and commented more easily.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

@daira moved the .cargo-checksum.json generation into a separate script.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

Hmm, after addressing comment from @ebfull, MacOS still breaks:

echo Preprocessing librustzcash...
Preprocessing librustzcash...
mkdir -p /Users/zcbbworker/macos-0/macos10-12/build/depends/work/build/x86_64-apple-darwin16.7.0/librustzcash/0.1-b76c4e1aec6 /Users/zcbbworker/macos-0/macos10-12/build/depends/work/build/x86_64-apple-darwin16.7.0/librustzcash/0.1-b76c4e1aec6/.patches-b76c4e1aec6
cd /Users/zcbbworker/macos-0/macos10-12/build/depends/patches/librustzcash; cp cargo.config /Users/zcbbworker/macos-0/macos10-12/build/depends/work/build/x86_64-apple-darwin16.7.0/librustzcash/0.1-b76c4e1aec6/.patches-b76c4e1aec6 ;
cd /Users/zcbbworker/macos-0/macos10-12/build/depends/work/build/x86_64-apple-darwin16.7.0/librustzcash/0.1-b76c4e1aec6;   mkdir .cargo && cp /Users/zcbbworker/macos-0/macos10-12/build/depends/work/build/x86_64-apple-darwin16.7.0/librustzcash/0.1-b76c4e1aec6/.patches-b76c4e1aec6/cargo.config .cargo/config && sed -i 's|CRATE_REGISTRY|/Users/zcbbworker/macos-0/macos10-12/build/depends/x86_64-apple-darwin16.7.0/vendored-sources|' .cargo/config
sed: 1: ".cargo/config": invalid command code .

I'll try streaming the patch instead.

@str4d str4d force-pushed the str4d:2231-depends-vendor-rust-crates branch from b6d9d35 to b490c0e Mar 21, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2018

⌛️ Trying commit b490c0e with merge 8605a21...

zkbot added a commit that referenced this pull request Mar 21, 2018

Auto merge of #3096 - str4d:2231-depends-vendor-rust-crates, r=<try>
depends: Explicitly download and vendor Rust dependencies

Closes #2231.
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

That worked. All builders successfully built the dependencies.

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2018

☀️ Test successful - pr-try
State: approved= try=True

awk -v OFS='":"' '{print $2, $1}' | # 'H(A) A' -> 'A":"H(A)'
sed 's|^|"|' | # 'A":"H(A)' -> '"A":"H(A)'
sed 's|$|"|' | # '"A":"H(A)' -> '"A":"H(A)"'
tr '\n' ',' | # Concatenate lines with commas

This comment has been minimized.

Copy link
@daira

daira Mar 23, 2018

Contributor

I think backslash-escaping within backticks is nonportable (POSIX allows the backslash sequence to be expanded early).

http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html#tag_23_02_06_03

This comment has been minimized.

Copy link
@daira

daira Mar 23, 2018

Contributor

This comment has been minimized.

Copy link
@str4d

str4d Mar 26, 2018

Author Contributor

I used backticks originally because of the Makefile dollar-symbol issue. I'll change it now.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

@zkbot try

(I expect this will fail due to PyFlakes warnings in master, as we now requiring the code to be PyFlakes-clean, but dependencies should build on all builders.)

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

⌛️ Trying commit f92064a with merge 5c6a828...

zkbot added a commit that referenced this pull request Mar 26, 2018

Auto merge of #3096 - str4d:2231-depends-vendor-rust-crates, r=<try>
depends: Explicitly download and vendor Rust dependencies

Closes #2231.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

💔 Test failed - pr-try

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

Two builders failed due to transient errors, but the other two failed due to PyFlakes warnings (yay!)

@str4d str4d added the review needed label Mar 27, 2018

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@str4d Hit an error that I saw in an earlier PR: https://github.com/zcash/zcash/pull/3117/commits

Makefile:154 recipe for target 'download-win' failed

After applying 3117 fix, I am able to test the above without failure:

make -C depends download

@mdr0id
Copy link
Contributor

left a comment

See last comment for fix related to missing 'download-win' target.

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

ACK from me!

@daira

daira approved these changes Mar 28, 2018

Copy link
Contributor

left a comment

utACK

@str4d str4d force-pushed the str4d:2231-depends-vendor-rust-crates branch from f92064a to 0adfdc9 Mar 28, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

Squashed commits at @daira's request.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2018

📌 Commit 0adfdc9 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2018

⌛️ Testing commit 0adfdc9 with merge a08ad2b...

zkbot added a commit that referenced this pull request Mar 28, 2018

Auto merge of #3096 - str4d:2231-depends-vendor-rust-crates, r=str4d
depends: Explicitly download and vendor Rust dependencies

Closes #2231.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing a08ad2b to master...

@zkbot zkbot merged commit 0adfdc9 into zcash:master Mar 28, 2018

1 check passed

homu Test successful
Details
find . -type f | # Get list of file paths
grep -v $1 | # Exclude Makefile hashes
grep -v .stamp_ | # Exclude Makefile stamps
sed 's|^./||' | # Remove leading ./

This comment has been minimized.

Copy link
@daira

daira Mar 29, 2018

Contributor

Spotted a bug post-merge: this is supposed to match only leading "./", but actually matches any leading character followed by "/" as the second character.

$ echo 'X/foo' |sed 's|^./||'  # incorrect
foo
$ echo 'X/foo' |sed 's|^[.]/||'  # correct
X/foo
$ echo './foo' |sed 's|^[.]/||'  # correct
foo

This comment has been minimized.

Copy link
@daira

daira Mar 29, 2018

Contributor

Similarly, the previous line should be grep -v '[.]stamp_' |.

This comment has been minimized.

Copy link
@str4d

str4d Mar 30, 2018

Author Contributor

Opened #3131.

@str4d str4d deleted the str4d:2231-depends-vendor-rust-crates branch Mar 30, 2018

@str4d str4d removed the review needed label Mar 30, 2018

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.