Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

docs: evmbin - Update Rust docs #10658

Merged
merged 14 commits into from
May 21, 2019
Merged

docs: evmbin - Update Rust docs #10658

merged 14 commits into from
May 21, 2019

Conversation

ltfschoen
Copy link
Contributor

No description provided.

@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@ltfschoen ltfschoen added the A0-pleasereview 🤓 Pull request needs code review. label May 14, 2019
@soc1c soc1c added the M3-docs 📑 Documentation. label May 14, 2019
@soc1c soc1c added this to the 2.6 milestone May 14, 2019
//!
//! Build Parity Ethereum from source, then start it with:
//! ```bash
//! cd evmbin && cargo build --release && cd ..
Copy link
Contributor

Choose a reason for hiding this comment

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

something like

cargo build -p evmbin

and is much simpler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've pushed a commit addressing your comment to simplify it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, i've pushed a commit fixing it now so it's:

//! ```bash
//! cargo build -p evmbin --release
//! ./target/release/parity-evm --help
//! ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that ok @soc1c ?

@ltfschoen
Copy link
Contributor Author

Credit to @tomusdrw who explained evmbin too me so I was able to add some of the comments

evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
//!
//! ## Usage
//!
//! Build Parity Ethereum from source, then start it with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it is not included in regular releases? That is a bit unusual so perhaps worth expanding on, like "The evmbin tool is not distributed with regular Parity Ethereum releases so you need to build it from source like so: …`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've pushed a commit fixing this, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you happy with the changes @dvdplm?

evmbin/src/main.rs Outdated Show resolved Hide resolved
//! Build Parity Ethereum from source, then start it with:
//! ```bash
//! cd evmbin && cargo build -p evmbin && cd ..
//! ./target/debug/parity-evm --help
Copy link
Collaborator

Choose a reason for hiding this comment

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

If built with --release the path becomes ./target/release/parity-evm (and we might want to suggest they copy the executable to, dunno, ~/bin?).

Copy link
Contributor

Choose a reason for hiding this comment

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

installing binaries is usually out of scope here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they're using macOS or Linux and they want it in their PATH, we could suggest that they create a symlink i.e. sudo ln -s <ABSOLUTE_PATH>/parity-ethereum/target/release/parity-evm /usr/local/bin, assuming that echo $PATH indicates that /usr/local/bin is in their path, and then they can run parity-evm --help from any directory.
I'd have to look into what steps are required for Windows.

Copy link
Contributor Author

@ltfschoen ltfschoen May 15, 2019

Choose a reason for hiding this comment

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

i've pushed a commit that informs the user the following: "If you wish to run parity-evm from any directory, then either copy the executable file itself to a directory that is in your PATH, or just create a symlink to it."

Is that ok @dvdplm @soc1c ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I'd avoid symlinks to the build directory because files therein are clobbered anytime I build. Maybe a user of evmbin doesn't build as often as I do, but who knows. I think the wording could just be: "if you wish to run parity-evm from any directory copy the executable from ./target/release to a directory in your $PATH, e.g. ~/bin or /usr/local/bin.".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i've pushed a commit with this change

Copy link
Contributor

Choose a reason for hiding this comment

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

again, I would like to emphasize that explaining users how to use binaries is out of scope for our documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soc1c I've pushed another commit removing the explanation about how to use binaries

evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm (other than nitpicks on some wording)

@sorpaas sorpaas merged commit 8998774 into master May 21, 2019
@sorpaas sorpaas deleted the luke-evmbin-docs branch May 21, 2019 17:42
dvdplm added a commit that referenced this pull request May 22, 2019
* master:
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
dvdplm added a commit that referenced this pull request May 23, 2019
* master:
  add_sync_notifier in EthPubSubClient holds on to a Client for too long (#10689)
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
ordian added a commit that referenced this pull request May 23, 2019
* master:
  add_sync_notifier in EthPubSubClient holds on to a Client for too long (#10689)
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
dvdplm added a commit that referenced this pull request Jul 3, 2019
…lly typed serde serialization using Rust structs (#10657)

* fix: Replace multirust with rustup wince multirust is deprecated

* docs: Update evmbin Rust docs and code comments

* WIP: Add Response struct. Initial step using serde to serialize instead of hardcoding with JSON

* fix: Update Response struct types to be string after formatting

* fix: Fix move out of borrowed content error by cloning informant

* refactor: Change from camelcase to snake case to fix linting errors

* restore: Restore some code since now covered in separate PR #10658

* restore: Restore original Rustdocs of evmbin

* WIP

* add Clone type

* add newlines to end of json files

* remove uml file that was unintentionally commited

* rename chain spec to state test JSON fle

* remove log. fix indentation

* revert: Restore indentation now handled by separate PR #10740

* remove state test json files as moved to PR #10742

* revert changes in info.rs since covered in PR #10742

* revert changes to main.rs since covered in PR #10742

* revert newlines back to master

* revert newlines back to master2

* refactor: Rename Response to TraceData

* fix: Remove Clone and replace with lifetimes. Update tests since not ordered

* refactor: Change all json! to typed serde

* docs: Update rustdocs. Remove fixme

* fix: Add missing semicolons from printf

* fix: Change style from unwrap to expect in evmbin/src/display/json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* fix: Change style from unwrap to expect in evmbin/src/display/std_json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* revert updating module comments as will be done in separate PR #10742 instead

* review-fix: Remove useless reference

* Remove unncessary use of format macro

* Update evmbin/src/display/json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/json.rs with serialization in set_gas success

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/json.rs with serialization in set_gas failure

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization in finish for state root

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization in before_test

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for state root

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for finish success

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for finish failure

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Rename structs and variables. Remove space. Simplify MessageInitial struct

* refactor: Captialize expect message

* revert to previous struct name TraceDataStateRoot

* refactor: Simplify variable for consistency

* Update accounts/ethstore/src/json/crypto.rs

Co-Authored-By: David <dvdplm@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M3-docs 📑 Documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants