iamtrask / Sonar

DEPRECATED - Decentralized Machine Learning Server (hosted on Blockchain)
Apache License 2.0
141 stars 49 forks source link

Testrpc build fix #16

Closed carrollgt91 closed 6 years ago

carrollgt91 commented 6 years ago

For some reason, the global install was failing to actually put the required executable in the node_modules/.bin directory. This fixes that issue by adding the package to Sonar's devDependenices instead.

Also, this will allow us to version control the version of testrpc we use in that image, leading to more stable builds.

anoff commented 6 years ago

Global installs don't end up in the local node_modules/ but a global one. You can just call them via their bin name. If the point is to lock-down the testrpc version I'd rather use npm install -g testrpc@1.2.3 than putting it in the dependencies. The devDependencies in this repository are used if you want to develop the smart contract - not for running the docker container.

Thoughts @axelhodler @TheOriginalAlex ?

axelhodler commented 6 years ago

LGTM, almost incorporated this as part of #12 but wanted to keep the change less invasive

since its not yet necessary to use the docker container i think the change is fine. but sooner or later the whole docker image for testing in-memory should probably be split off in a separate repo and then used by all the projects interested in a in-memory blockchain. meaning testrpc will leave the package.json again

carrollgt91 commented 6 years ago

Yeah, I imagine that an openmined testrpc repo isn't too far in the distance, since Sonar is not the only project using this docker image anymore. Thanks guys!

ghost commented 6 years ago

LGTM. 👍