replaces path to .asar.unpacked if cmd includes .asar/node_modules #155

thisconnect · 2017-06-22T09:14:17Z

js-ipfsd-ctl will not work inside an asar container, see #94.

A possible solution is to exclude the ipfs binary from asar, with the unpackDir option

electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep

(EDITED: changed go-ipfs-dep/** to go-ipfs-dep )

on MacOS: this will package everything expect go-ipfs-dep into <appname>.asar
and put go-ipfs-dep into <appname>.asar.unpacked/node_modules/go-ipfs-dep

This change request adds a check if .asar/ is part of the path and changes it to .asar.unpacked/

Edit:

How to test this change

Assuming you have a project using ipfsd-ctl, electron and electron-packager.

Install https://github.com/thisconnect/js-ipfsd-ctl/tree/asar-unpacked

npm i thisconnect/js-ipfsd-ctl#asar-unpacked

build electron with --asar.unpackDir=node_modules/go-ipfs-dep

electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep

Test if your app works and if everything (but go-ipfs-dep) is bundled in an .asar file

daviddias · 2017-06-24T11:15:35Z

@thisconnect I'm not sure if I understand all the implications here. If go-ipfs doesn't get bundled, then how would a standalone electron app that wants to use it work?

thisconnect · 2017-06-24T13:17:28Z

electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep does only exclude from the asar archive, but still bundle the files in the app. On MacOS I get

MyApp.app/Contents/Resources/app.asar
MyApp.app/Contents/Resources/app.asar.unpacked/node_modules/go-ipfs-dep

This PR checks if the path to the ipfs binary points to inside an asar archive and changes the path to asar.unpacked. It tests for .asar/node_modules which probably will not work on windows.

With this change It successfully bundle MyApp into an asar archive and am able to start the ipfsd-ctl deamon, that is in app.asar.unpacked.

At this point I think it should be done in the findIpfsExecutable function https://github.com/thisconnect/js-ipfsd-ctl/blob/4453ea7caed94a00cac54d57c1e60d1e6420c1d7/src/node.js#L19

related docs https://electron.atom.io/docs/tutorial/application-packaging/#executing-binaries-inside-asar-archive

An other solution would be to be able to configure the path to the ipfs binary.

thisconnect · 2017-06-24T13:32:12Z

The configuration could be done on as api or environment variable. As a ipfsd-ctl consumer I want to just use the api and not write my code differently for a packaging detail.

I am unsure if configuring the path with an env variable is possible and a good idea :/

daviddias · 2017-06-24T14:22:53Z

Got it. I'm afraid I haven't used enough electron to know the full breadth of issues, but this PR doesn't seem to break our existing use cases so I'm fine trying it out :)

Could you add documentation of the issue and how to circumvent it with the packaging command you mention here so that others can try it too?

@kyledrake, you've been hacking on electron and going through the same issues, mind sharing your thoughts here?

thisconnect · 2017-06-24T16:23:02Z

Please do NOT merge this yet, it may not work on windows, due to forward slash. Not sure if that works with Node 4.x / npm 2.x when modules are not deduped.

I have not tried electron-builder yet. With electron-builder asar is enabled by default, see https://github.com/electron-userland/electron-builder/wiki/Options#AsarOptions
also it seems electron-builder detects binaries and unpacks them automatically, though I am not sure how that works. I am going to try this soon.

edit: moved howto to PR description

thisconnect · 2017-07-02T18:42:47Z

Ok tested thisconnect/js-ipfsd-ctl#asar-unpacked on windows 10.

for windows

added os.platform check to set the executable filename to ipfs.exe
electron-packager wildcard (glob?) seems to work differently on windows, changed
electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep/**
to
electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep
so that it includes the dir and the binary

Please review https://github.com/ipfs/js-ipfsd-ctl/pull/155/files

daviddias · 2017-08-29T12:52:46Z

@thisconnect is this ready for review and merge?

thisconnect · 2017-08-30T05:19:17Z

@diasdavid yes please.

also please have a look at this line, https://github.com/ipfs/js-ipfsd-ctl/pull/155/files#diff-6ff379484cbabad48301d485db111c08R32 that should have been a different PR.

in the meantime the power port of my windows laptop melted. Do you use appveyor for windows CI?

daviddias · 2017-08-30T06:58:18Z

@thisconnect just enabled appveyor for this repo. Could you add the config file (Tutorial, Great example)

https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl-a9ywu

daviddias · 2017-08-30T07:02:22Z

Also, just added you to the JS team. You can now create branches on the main repo and PR directly from them. This helps collaboration between multiple people

https://github.com/orgs/ipfs/teams/javascript-team/members 🌟

thisconnect · 2017-08-31T06:55:10Z

Thanks @diasdavid I'll find time over the weekend and will create a branch for appveyor.

daviddias · 2017-08-31T08:58:22Z

Ok, I'll be more tranquil to merge this PR when I see the tests passing in Windows and in Linux CI :)

…d (tested macos and win32)

daviddias

@thisconnect thank you for keeping on pushing on this front :) Can't wait to merge this PR!

daviddias · 2017-09-04T08:48:03Z

src/node.js

+  // electron-packager ./ --asar.unpackDir=node_modules/go-ipfs-dep
+  if (appRoot.includes(`.asar${path.sep}`)) {
+    appRoot = appRoot.replace(`.asar${path.sep}`, `.asar.unpacked${path.sep}`)
+  }


Could you add this to the README too? It is fine to leave it here, but I want to make sure it this note is searchable and linkable to

thisconnect · 2017-09-04T12:17:27Z

There is a failing test on windows https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl/build/21/job/ohabcvtp3jtlsgoa#L1610 ,
this to be the same issue as ipfs-inactive/js-ipfs-http-client#408

daviddias · 2017-09-04T12:42:54Z

@thisconnect ipfs-inactive/js-ipfs-http-client#408 shouldn't be an issue of this module specifically. Is there another test you can add to check that the module works?

thisconnect · 2017-09-04T13:50:18Z

@diasdavid I have never built electron within CI, it should be possible but might take a long time. Not sure if it is worth right now.

I tried to add some info in the readme and a minimal example in examples/electron-asar, please have a look

thisconnect · 2017-09-04T13:55:42Z

The "checks response of ipfs.util.addFromFs for windows" now tests too much.

Do you prefer

A) the deep equal that should cover everything about the response https://github.com/thisconnect/js-ipfsd-ctl/blob/46c422ebf1f5b11acecf81da74a553fbab4093d5/test/index.spec.js#L394-L406

B) or remove it and keep the rest https://github.com/thisconnect/js-ipfsd-ctl/blob/46c422ebf1f5b11acecf81da74a553fbab4093d5/test/index.spec.js#L408-L418

C) or keep it as it was before

EDIT: other than that I think it should be ready, unfortunately my windows laptop's power supply burned so I can't test on windows.

daviddias · 2017-09-04T13:59:17Z

@thisconnect given that addFromFs has issues in Windows, I don't believe it to be a good test case to have. ~~Better to skip (add a .skip) it for now.~~

Update: Check process.env and if Windows skip that test with a comment describing that is waiting on the fix for js-ipfs-api

daviddias · 2017-09-04T14:04:49Z

test/index.spec.js

@@ -390,10 +390,32 @@ describe('daemons', () => {
        if (err) throw err


see: #155 (comment)

Here is an example: https://github.com/ipfs/npm-go-ipfs-dep/blob/master/src/index.js#L47-L60

As it is a temporary skip, is it ok if I just os.platform() === 'win32' ?

https://nodejs.org/api/os.html#os_os_platform

I don't get https://github.com/ipfs/npm-go-ipfs-dep/blob/master/src/index.js#L47-L60 sorry!! :( it also depends on another module (go-platform) and file https://github.com/ipfs/npm-go-ipfs-dep/blob/master/src/check-support.js

daviddias

Woot! Will just wait on CI!

thisconnect · 2017-09-04T14:34:51Z

Not sure what the last one is all about https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl/build/24/job/t4r5euk57pms9d20#L1610

daviddias · 2017-09-04T14:37:36Z

Seems like a go-ipfs thing, @Kubuxu ?

thisconnect · 2017-09-04T14:44:58Z

https://github.com/thisconnect/js-ipfsd-ctl/blob/f3e49d54ecb143d130812dfc07ebd0788ae5aa72/test/index.spec.js#L438 checks for a repo.lock file in index.spec.js , something before did not close correctly?

Kubuxu · 2017-09-04T14:56:21Z

Nothing changed in go-ipfs that could cause it. How does shutting down work? Does it wait for go-ipfs to shutdown? Is it station timeout or does it poll the process?

daviddias · 2017-09-04T15:04:23Z

Nothing changed in go-ipfs that could cause it. How does shutting down work?

It is the first time we are enabling Windows testing in this module, so really anything is worth ;)

Sounds like that go-ipfs never really deleted the lock file but in windows (such as OS X or Linux) it just gives 0 frambroises and continues. Here, since we check for it, we actually saw it is there.

@thisconnect let's skip that check if in Windows as well :) This isn't a real issue.

Thanks for jumping in too @Kubuxu :)

Kubuxu · 2017-09-04T15:08:18Z

It might be Windows, Windows is a bit neglected so it is possible that this is an issue.

daviddias · 2017-09-04T15:27:23Z

test/index.spec.js

+          if (fs.existsSync(path.join(dir, 'api'))) {
+            cb(new Error('api file not removed'))
+          }
+          cb()


I didn't mean to skip the whole test, just to skip the assertion for repo.lock

daviddias · 2017-09-04T15:27:40Z

test/index.spec.js

-          cb(new Error('api file not removed'))
+    // skip on windows
+    // https://github.com/ipfs/js-ipfsd-ctl/pull/155#issuecomment-326983530
+    if (isWindows) it.skip('start and stop')


add curly {}

thisconnect · 2017-09-04T15:29:00Z

there was travis fail Uncaught TypeError: Cannot read property 'constructor' of null https://travis-ci.org/ipfs/js-ipfsd-ctl/builds/271713202?utm_source=github_status&utm_medium=notification

thisconnect · 2017-09-04T15:43:32Z

api file assert has to go too https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl/build/26/job/gubdael6q0fl8fxu#L1685

daviddias · 2017-09-04T15:55:11Z

test/index.spec.js

+          }
+          if (fs.existsSync(path.join(dir, 'api'))) {
+            cb(new Error('api file not removed'))
+          }


thisconnect · 2017-09-04T15:55:56Z

https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl-a9ywu/build/27/job/9ocd4i7lcflnxh6c#L129 :(

thisconnect mentioned this pull request Jun 22, 2017

Doesnt work in Electron apps that have used .asar to package themselves #94

Closed

thisconnect changed the title ~~WIP: replaces path to .asar.unpacked if cmd includes .asar/node_modules~~ replaces path to .asar.unpacked if cmd includes .asar/node_modules Jul 3, 2017

daviddias added the status/in-progress In progress label Jul 3, 2017

daviddias assigned thisconnect Aug 30, 2017

daviddias mentioned this pull request Sep 2, 2017

adds .appveryor.yml #162

Merged

tcme added 9 commits September 2, 2017 11:40

chrore: adds .appveryor.yml

e9ac8b0

replaces path to .asar.unpacked if cmd includes .asar/node_modules

c641866

moves asar detection and win fix

03ea8d5

should be appRoot

60fa938

forgot os

a6b1347

lints

9f53ce8

updates comment electron-packager with asar.unpackDir without wildcar…

1b9dcf8

…d (tested macos and win32)

removes .exe in executable path on windos

279f58f

tests ipfs-api version response length and first item

3fc9347

daviddias self-requested a review September 4, 2017 08:47

daviddias requested changes Sep 4, 2017

View reviewed changes

thisconnect added 2 commits September 4, 2017 10:51

checks response of ipfs.util.addFromFs for windows

135dca4

checks response of ipfs.util.addFromFs for windows again

46c422e

thisconnect added 2 commits September 4, 2017 15:40

adds info about packaging with asar and electron-asar example

92841df

disables eslint no-console warning for examples

5190a48

cleanup, to be ready for releasing

32e8200

daviddias reviewed Sep 4, 2017

View reviewed changes

skips addFromFs on win32

f3e49d5

daviddias approved these changes Sep 4, 2017

View reviewed changes

skips multiple start stop test on windows

0510863

daviddias requested changes Sep 4, 2017

View reviewed changes

skips only repo.lock assertion

d74578b

untests api file on windows

d1a7bdf

daviddias reviewed Sep 4, 2017

View reviewed changes

test/index.spec.js

}

if (fs.existsSync(path.join(dir, 'api'))) {

cb(new Error('api file not removed'))

}

Copy link

Member

daviddias Sep 4, 2017

Choose a reason for hiding this comment

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

👍

daviddias changed the base branch from master to windows September 4, 2017 15:56

daviddias merged commit abed2e4 into ipfs:windows Sep 4, 2017

		@@ -390,10 +390,32 @@ describe('daemons', () => {
		if (err) throw err

replaces path to .asar.unpacked if cmd includes .asar/node_modules #155

replaces path to .asar.unpacked if cmd includes .asar/node_modules #155

Conversation

thisconnect commented Jun 22, 2017 • edited Loading

How to test this change

daviddias commented Jun 24, 2017

thisconnect commented Jun 24, 2017 • edited Loading

thisconnect commented Jun 24, 2017

daviddias commented Jun 24, 2017

thisconnect commented Jun 24, 2017 • edited Loading

thisconnect commented Jul 2, 2017 • edited Loading

daviddias commented Aug 29, 2017

thisconnect commented Aug 30, 2017

daviddias commented Aug 30, 2017 • edited Loading

daviddias commented Aug 30, 2017

thisconnect commented Aug 31, 2017

daviddias commented Aug 31, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisconnect commented Sep 4, 2017

daviddias commented Sep 4, 2017

thisconnect commented Sep 4, 2017

thisconnect commented Sep 4, 2017 • edited Loading

daviddias commented Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

thisconnect commented Sep 4, 2017

daviddias commented Sep 4, 2017

thisconnect commented Sep 4, 2017 • edited Loading

Kubuxu commented Sep 4, 2017

daviddias commented Sep 4, 2017 • edited Loading

Kubuxu commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisconnect commented Sep 4, 2017

thisconnect commented Sep 4, 2017

Choose a reason for hiding this comment

thisconnect commented Sep 4, 2017

thisconnect commented Jun 22, 2017 •

edited

Loading

thisconnect commented Jun 24, 2017 •

edited

Loading

thisconnect commented Jun 24, 2017 •

edited

Loading

thisconnect commented Jul 2, 2017 •

edited

Loading

daviddias commented Aug 30, 2017 •

edited

Loading

thisconnect commented Sep 4, 2017 •

edited

Loading

daviddias commented Sep 4, 2017 •

edited

Loading

thisconnect commented Sep 4, 2017 •

edited

Loading

daviddias commented Sep 4, 2017 •

edited

Loading