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

Pass parameter paths as native strings to librustzcash #3633

Merged
merged 3 commits into from Oct 27, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Oct 27, 2018

Fixes a user-reported crash when starting on Windows with a Cyrillic non-UTF-8 locale.

Migrate to current librustzcash
The only upstream change relative to the previous commit is that the
various Zcash-specific dependencies have been pulled into a cargo
workspace. The dependecies in the workspace use the same commits as the
crates we had previously vendored.

The patches are necessary to handle the fact that cargo requires that
dev dependencies are available even if not used, and we would otherwise
need to vendor all the underlying crates.
@mdr0id

mdr0id approved these changes Oct 27, 2018

Copy link
Contributor

mdr0id left a comment

Tested ACK:

Alright, so that Windows 1803 patch has the update to allow users to manually check the beta box option for Use Unicode UTF-8 for worldwide language support . After update/restart Winzec will still crash (atleast for me). I then ran the zcashd.exe by itself and it turns out the zcash.conf was just missing. Created a fresh zcash.conf with addnode=mainnet.z.cash, restarted Winzec, keys were pulled /verified, and started gracefully using username: йохн2

@str4d str4d force-pushed the str4d:params-path-encoding branch from 978f60c to ee3fa70 Oct 27, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 27, 2018

Force-pushed to update the commit to use the correct librustzcash commit hash (now that zcash/librustzcash#45 has been merged).

@ebfull

ebfull approved these changes Oct 27, 2018

@str4d str4d added this to Merge Queue in Zcashd Team Oct 27, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 27, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

📌 Commit ee3fa70 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

⌛️ Testing commit ee3fa70 with merge 9186a09...

zkbot added a commit that referenced this pull request Oct 27, 2018

Auto merge of #3633 - str4d:params-path-encoding, r=str4d
Pass parameter paths as native strings to librustzcash

Fixes a user-reported crash when starting on Windows with a Cyrillic non-UTF-8 locale.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 27, 2018

Rust dependency issue.

Building librustzcash...
mkdir -p /home/zcbbworker/latent-0/debian8/build/depends/work/build/x86_64-unknown-linux-gnu/librustzcash/0.1-15ffc5364ae/.
cd /home/zcbbworker/latent-0/debian8/build/depends/work/build/x86_64-unknown-linux-gnu/librustzcash/0.1-15ffc5364ae/.; PATH="/home/zcbbworker/latent-0/debian8/build/depends/x86_64-unknown-linux-gnu/native/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"   cd librustzcash && cargo build --frozen --release        && cd ..
/bin/sh: 1: cargo: not found
funcs.mk:257: recipe for target '/home/zcbbworker/latent-0/debian8/build/depends/work/build/x86_64-unknown-linux-gnu/librustzcash/0.1-15ffc5364ae/./.stamp_built' failed
make: *** [/home/zcbbworker/latent-0/debian8/build/depends/work/build/x86_64-unknown-linux-gnu/librustzcash/0.1-15ffc5364ae/./.stamp_built] Error 127
make: Leaving directory '/home/zcbbworker/latent-0/debian8/build/depends'
@daira

daira approved these changes Oct 27, 2018

Copy link
Contributor

daira left a comment

utACK modulo the comment about rust_crates. Note that this has no tests for non-ASCII paths; please file a ticket to add them.

@@ -3,7 +3,6 @@ rust_crates := \
crate_aesni \
crate_aes_soft \
crate_arrayvec \
crate_bellman \

This comment has been minimized.

@daira

daira Oct 27, 2018

Contributor

I thought rust_crates was supposed to be the transitive closure of crate dependencies?

This comment has been minimized.

@str4d

str4d Oct 27, 2018

Contributor

It is the set of crates that are dependencies fetched from remote repositories. The removed dependencies are included locally via the cargo workspace, and thus don't need to be vendored,

@daira daira referenced this pull request Oct 27, 2018

Closed

WinZec 2.0.1 does not work #3635

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 27, 2018

Building locally, I get the following error a few times:

   Compiling aes v0.2.0
error[E0658]: `cfg(target_feature)` is experimental and subject to change (see issue #29717)
  --> /projects/zcash/depends/x86_64-unknown-linux-gnu/vendored-sources/aes/src/lib.rs:46:5
   |
46 |     target_feature="aes", target_feature = "sse2",
   |     ^^^^^^^^^^^^^^^^^^^^

...

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0658`.
error: Could not compile `aes`.
warning: build failed, waiting for other jobs to finish...
error: build failed

Build librustzcash package without changing directory
This ensures that the depends system's custom PATH is applied correctly,
and the pre-build Rust binaries are accessible.
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 27, 2018

Pushed a commit that fixes the CI problem. @radix42 confirmed the fix locally.

@bitcartel that is likely due to the bug I just fixed, which mean that your system Rust installation was being used. If it's an older version than 1.28.0, I would not be surprised there are errors.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 27, 2018

Build issue now resolved.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

📌 Commit 262cf38 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

⌛️ Testing commit 262cf38 with merge 11fa127...

zkbot added a commit that referenced this pull request Oct 27, 2018

Auto merge of #3633 - str4d:params-path-encoding, r=bitcartel
Pass parameter paths as native strings to librustzcash

Fixes a user-reported crash when starting on Windows with a Cyrillic non-UTF-8 locale.
@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Oct 27, 2018

re-ACK

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 27, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 11fa127 to master...

@zkbot zkbot merged commit 262cf38 into zcash:master Oct 27, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from Merge Queue to Released (Merged in Master) Oct 27, 2018

@str4d str4d added this to the v2.0.2 milestone Nov 16, 2018

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