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

Allow installing package from git #51

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

vsbogd
Copy link
Member

@vsbogd vsbogd commented Oct 12, 2018

This is to allow referring from dependent projects (such as snet-cli or snet-daemon) to latest github version without publishing it to the npm package registry.

To do so npm install hook is added which runs npm run package-npm command. .npmrc file is used to distinguish local npm install and one running from dependent project. When package is installed as dependency this file is not downloaded because it is not listed in files section of the package.json. Thus package-npm.js which runs from dependent project installs abi to package root directory and when package-npm.js is started from cloned platform-contracts repository it copies files to build/npm-module.

@astroseger
Copy link
Collaborator

astroseger commented Oct 14, 2018

I think that putting ABI files to github was an error at the first place. If we will update them at each commit then size of the repo will grow without a limit. It is not a right way of using git. So I would propose to remove build/contracts/*.ABI at all. @tiero and @vforvalerio87, I know that it was some reason to put ABI at github, but there is, certainly, should be an other way to solve these problems, without misusing git.
So, in light of above, I don't like this solution because it require putting ABI at github. I think that, for now, the way with separate "dev" installation script, which takes platform-contract ABI from already installed contracts would be better (see scripts/install_dev.py in snet-cli).

But:

  1. In the future, @ksridharbabuus will probably change installation procedure in any way.
  2. We don't have much time now.

So if you already have an installation procedure for daemon which use this solution, then I propose:

  1. merge this pull requiest (but please update ABI first, I'm going to merge pull request Switch expiration to blocknumber in MultiPartyEscrow #50 now)
  2. Create an issue about removing ABI from github, and update installation process for the whole project

Add local variable into .npmrc to separate local and nonlocal install.
List all files to be downloaded when package is installed as dependency.
Add postinstall script to check local variable and run compilation and
packaging if package is installed as dependency.
@vsbogd vsbogd force-pushed the install-package-from-git branch from 5b2b5f4 to af9a198 Compare October 15, 2018 08:36
@vsbogd
Copy link
Member Author

vsbogd commented Oct 15, 2018

Then I would propose compiling contracts after installing npm package as dependency. It will not require adding compiled contracts into repository.

Separate dev script is not unified with npm which is used to install dependencies at the moment. It additionally requires changes in .circleci configuration, additional manual instructions (how to build with and without dev dependency). And these things should be done in each repo which uses platform-contracts.

This change instead requires only pointing on new dependency in npm configuration:

$ git diff resources/blockchain/package.json
diff --git a/resources/blockchain/package.json b/resources/blockchain/package.json
index 6c21d60..5bc7c4b 100644
--- a/resources/blockchain/package.json
+++ b/resources/blockchain/package.json
@@ -3,7 +3,7 @@
     "bip39": "2.5.0",
     "fs-extra": "5.0.0",
     "ganache-cli": "6.1.6",
-    "singularitynet-platform-contracts": "0.2.1",
+    "singularitynet-platform-contracts": "github://singnet/platform-contracts#af9a198a4fd519f45944ace3f636931fbdfa5dec",
     "singularitynet-token-contracts": "2.0.0",
     "truffle": "4.1.13",
     "truffle-contract": "3.0.6",

@vsbogd
Copy link
Member Author

vsbogd commented Oct 15, 2018

As @astroseger proposed, I am merging it for now, and opening issue to discuss and find a better solution #53

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.

2 participants