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

Upgrade js-ipfsd-ctl to use bin-wrapper #78

Closed
daviddias opened this issue May 18, 2016 · 20 comments
Closed

Upgrade js-ipfsd-ctl to use bin-wrapper #78

daviddias opened this issue May 18, 2016 · 20 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue

Comments

@daviddias
Copy link
Member

Our friends from Akasha made a great wrapper around the IPFS binary (https://github.com/AkashaProject/ipfs-connector/blob/master/src/IpfsBin.ts) that is way more portable when our current approach in js-ipfsd-ctl.

It uses https://www.npmjs.com/package/bin-wrapper which enables the app to select the binary for the right arch seamlessly (a issue we were hitting on electron).

Let's move to the bin-wrapper approach :)

@daviddias daviddias added help wanted Seeking public contribution on this issue exp/novice Someone with a little familiarity can pick up labels May 18, 2016
@haadcode
Copy link
Member

What were the issues with Electron?

@daviddias
Copy link
Member Author

@haadcode we were still working on how to get the right binary on Windows, once the electron app got 'packaged'

@dignifiedquire
Copy link
Member

Not only windows, any other OS than the one the packaging happens on. It is not entirely clear if this will solve all our issues, but most importantly I think these changes actually need to happen in https://github.com/ipfs/npm-go-ipfs

@haadcode
Copy link
Member

Am I understanding this correctly: the problem is that when packaging, only the package for the platform you're packaging on gets pulled in?

@haadcode
Copy link
Member

On that note, I've been wanting a local cache for the downloaded go-ipfs binary for a long time, so that the package doesn't get downloaded every time.

@daviddias
Copy link
Member Author

@dignifiedquire I would even argue that npm-go-ipfs, go-ipfs-dep and js-ipfsd-ctl could all be just one module with binwrapper

@daviddias
Copy link
Member Author

This required go-ipfs 0.4.3 to be released (Jun 29) and available through dist.ipfs.io. However, the work to make this migration can start immediately

@haadcode
Copy link
Member

haadcode commented Jul 4, 2016

If I understand bi-wrapper correctly, it'll download the correct bin at runtime (if needed, I suppose there's some caching happening), is that correct?

If so, would we use it to package different (platform) versions of js-ipfs-ctl or would it work so that in my program, I require js-ipfs-ctl which in turn would download the correct go-ipfs version at runtime?

My main concern here is that if bin-wrapper requires programs to download the correct bin at runtime (as opposed to build time), that'll be a horrible solution for a lot of offline first programs. And in my book, that's an absolute no go.

If it doesn't work as described above, or bin-wrapper downloads the bins at build time, then we're fine.

Can someone clarify? @diasdavid @dignifiedquire @kenshyx

@dignifiedquire
Copy link
Member

Using bin-wrapper the download will happen the first time it is run, and is cached onward. If you want to create specific bundles packing the binary you should generate them yourselfs using https://www.npmjs.com/package/go-ipfs and pass the location of them to ipfsd-ctl using IPFS_PATH.

@haadcode
Copy link
Member

haadcode commented Jul 4, 2016

Using bin-wrapper the download will happen the first time it is run, and is cached onward.

Ok, got it. This answers my questions re. how bin-wrapper works.

If you want to create specific bundles packing the binary you should generate them yourselfs using https://www.npmjs.com/package/go-ipfs and pass the location of them to ipfsd-ctl using IPFS_PATH.

Not sure what you mean here.

The question of how it'll work with js-ipfsd-ctl is still open. Would a program that uses js-ipfsd-ctl need an internet connection to download ipfs binary before it can work/run?

@haadcode
Copy link
Member

haadcode commented Jul 4, 2016

Ok, had a call with @dignifiedquire and my questions got answered.

Basically, bin-wrapper will allow us to have the same build/workflow as now. If it becomes a problem for users to figure out the packaging (mainly w/ Electron), we'll address that in the build tools (eg. Gulp, Grunt).

All good, SGTM!

@daviddias
Copy link
Member Author

daviddias commented Jul 4, 2016

@JGAntunes is working on getting this migrated to bin-wrapper :)

@daviddias
Copy link
Member Author

-> Follow on here #89

@jbenet
Copy link
Member

jbenet commented Aug 22, 2016

It is critical that everything required to run should be downloaded on
install
. Please think through the failure scenarios to understand why it's
a non starter to download go-ipfs on first run. I can enumerate them later
if you can't find any, but it's an important distributed systems lesson to
understand intuitively why this is a bad dsys design decision, and it won't
sink in if I just describe them all the time. ("Exercise for the reader.")

(Run it on install if you have to.)
On Mon, Aug 15, 2016 at 16:59 David Dias notifications@github.com wrote:

-> Follow on here #89 #89


You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub
#78 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoQ_dtatrVc6YplvgABbAIXdcGBYZks5qgNMngaJpZM4IhOPN
.

@haadcode
Copy link
Member

It is critical that everything required to run should be downloaded on
install
.

This is exactly why I started this issue. As far as I understand, that should be the case. I was assured of that.

@diasdavid @dignifiedquire can you, once more, confirm this just to make sure we're all on the same page and those with concerns re. this can chill back? :)

@daviddias
Copy link
Member Author

It is up to the user, but just to clarify

  • by default: npm install won't trigger the ipfs bin download
  • you can add the above feature with a post-install script on your package.json
  • by default: the ipfs bin will be downloaded the first time the code is ran and cached

So, for e.g: in a traditional Mac OS X app install, a user would get the .dmg, drag the .app, click the .app, it would open the first time dialog screen (traditionally the install/first time configure) and when that is happening, it is downloading the binary for the arch.

This is what you get out of the box ^^ However, It doesn't stop you from creating several bundles with the different ipfs bin.

@jbenet
Copy link
Member

jbenet commented Aug 25, 2016

  • So "no, it does not always automatically download go-ipfs when you npm install <module>". This is bad.
  • We can add the post-install script ourselves to our own module, the same way that it works now. Can't we?
  • Again, "download on first run" is an extremely brittle design decision, from a distributed systems perspective, and we should NOT default to this. This is like handing people a knife blade first.
  • If you think allowing users the flexibility is paramount, flip it, so that by default it will always download go-ipfs by just adding it as a dependency (no need to add a post-install hook).

@JGAntunes
Copy link
Member

@diasdavid @jbenet we can create a postinstall script which will run right after npm install and will download the respective binary.

@jbenet
Copy link
Member

jbenet commented Aug 26, 2016

Great, let's do that. (I believe it's how it works now). Thank you.
On Thu, Aug 25, 2016 at 04:34 João Antunes notifications@github.com wrote:

@diasdavid https://github.com/diasdavid @jbenet
https://github.com/jbenet we can create a postinstall script which will
run right after npm install and will download the respective binary.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#78 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoUCZcUaGvFtbSzz-BriyN-lQKAJdks5qjVOJgaJpZM4IhOPN
.

@daviddias
Copy link
Member Author

This is no longer required with the updated https://github.com/ipfs/npm-go-ipfs-dep

@daviddias daviddias removed the status/ready Ready to be worked label Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue
Projects
None yet
5 participants