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

FIX: caliper bind command now compatible with Windows OS #1127

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

fransotodev
Copy link
Contributor

Signed-off-by: FranSoto7 [email protected]

npx caliper bind command wouldn't work on a Windows machine, since the command for npm would be "npm.cmd" instead of just "npm".

Added a comprobation in runtime to check if the OS is Windows, otherwise it works like before.

@aklenik
Copy link
Contributor

aklenik commented Feb 26, 2021

@fransotodev Thanks for the improvement! Currently, there's a blocking issue in the Fabric CI, which will be fixed soon, so your PR checks can pass.

On a different note, did it take anything special to run Caliper on Windows? It would be nice if you could share your experience, for example, in the form of some documentation :)

@fransotodev
Copy link
Contributor Author

Hi @aklenik,
Running it on windows was pretty straightforward, I just followed the official docs and the only problem I found was that it couldn't launch the npm command from the caliper binder module.

That said, I used caliper with an Ethereum Smart Contract deployed using truffle and a ganache blockchain and I had to tweak a bit the Ethereum adapter module. Nothing to big at all, but want to check it out again to give a proper solution.

I will open another PR once I have a good solution for that one (I hardcoded something 😅) or open an issue with the things I did. Actually I wanted to commit that change first to see how all the process of contributing was (first time contributing 😄)

@aklenik
Copy link
Contributor

aklenik commented Feb 27, 2021

@fransotodev Awesome! Feel free to raise an issue about the Ethereum adapter describing the limitations, and raise the PR for it. I'm sure we can incorporate it in the codebase. And welcome to the Caliper contributors 🥇

@nklincoln
Copy link
Contributor

@aklenik - is this good to go in now?

@aklenik
Copy link
Contributor

aklenik commented Mar 16, 2021

@nklincoln Haven't had the chance to test it on Windows, but it's compatible with the current platform support.
If the changes were enough for @fransotodev , then we can declare experimental Windows support :)

So I'll give the green stamp for the PR.

Copy link
Contributor

@aklenik aklenik left a comment

Choose a reason for hiding this comment

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

LGTM

@fransotodev
Copy link
Contributor Author

Hi guys,

I said earlier in this PR that i had some problems with the Ethereum adapter, but those were my bad writing the workloads, nothing to do with the adapter or Windows.

So I didn't have any other problem, this little change was enough ^^

I'm good to merge and close this one.

@aklenik aklenik merged commit afc4bb6 into hyperledger-caliper:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants