Add support for prebuilt asars #823

jsg2021 · 2018-04-10T18:22:12Z

I have read the contribution documentation for this project.
I agree to follow the code of conduct that this project follows, as appropriate.
The changes are appropriately documented (if applicable).
The changes have sufficient test coverage (if applicable).
The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:
I implemented the --asar.filename option referenced in #161

welcome · 2018-04-10T18:22:15Z

Thanks for opening a pull request!

Here are some highlighted action items that will help get it across the finish line, from the
pull request guidelines:

Follow the JavaScript coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes in NEWS.md and other docs.
Include tests when adding/changing behavior.

Development and triage is community-driven, so please be patient and we will get back to you as soon as we can.

codecov · 2018-04-10T18:40:58Z

Codecov Report

Merging #823 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #823   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         622    643   +21     
=====================================
+ Hits          622    643   +21

Impacted Files	Coverage Δ
ignore.js	`100% <100%> (ø)`	⬆️
platform.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5e0f62...2183e42. Read the comment docs.

malept

Thank you for the pull request! This is an excellent starting point. I've made several comments that should hopefully move this PR in the right direction. Let me know if there's anything I need to clarify.

malept · 2018-04-10T18:35:01Z

.editorconfig

+
+[package.json]
+indent_style = space
+indent_size = 2


If we're going to add this, it should be in a separate PR.

sorry about that, I didn't mean to add that. I was trying to follow the current style and that file made my editor honor it.

malept · 2018-04-10T18:35:50Z

NEWS.md

@@ -4,6 +4,8 @@

 [Unreleased]: https://github.com/electron-userland/electron-packager/compare/v12.0.0...master

+* Added `asar.filename` to support pre-built asar files.


Could you please adjust this to conform to the Keep a Changelog format?

malept · 2018-04-10T18:36:32Z

docs/api.md

 - `ordering` (*String*): A path to an ordering file for packing files. An explanation can be found on the [Atom issue tracker](https://github.com/atom/atom/issues/10163).
 - `unpack` (*String*): A [glob expression](https://github.com/isaacs/minimatch#features), when specified, unpacks the file with matching names to the `app.asar.unpacked` directory.
 - `unpackDir` (*String*): Unpacks the dir to the `app.asar.unpacked` directory whose names exactly or pattern match this string. The `asar.unpackDir` is relative to `dir`.

  Some examples:

+  - `asar.filename = './dist/app.asar'` will short-circuit creating the asar, and just copy that one into the electron bundle.


I don't think there needs to be an example for this, it should be self-explanatory.

malept · 2018-04-10T18:37:02Z

docs/api.md

@@ -183,12 +183,14 @@ is not restricted to the official set if [`download.mirror`](#download) is set.
 *Boolean* or *Object* (default: `false`)

 Whether to package the application's source code into an archive, using [Electron's archive format](https://github.com/electron/asar). Reasons why you may want to enable this feature are described in [an application packaging tutorial in Electron's documentation](http://electron.atom.io/docs/v0.36.0/tutorial/application-packaging/). When the value is `true`, pass default configuration to the `asar` module. The configuration values listed below can be customized when the value is an `Object`. Supported parameters include, but are not limited to:
+- `filename` (*String*): circumvents the asar generation and lets you specify your own pre-built asar file/directory.


The description should start with "A path to", like ordering below it.

malept · 2018-04-10T18:38:08Z

test/_setup.js

@@ -67,13 +67,14 @@ function npmInstallForFixture (fixture) {
        return true
      } else {
        console.log(`Running npm install in fixtures/${fixture}...`)
-        return exec('npm install --no-bin-links', {cwd: fixtureDir})
+        return exec('npm install --no-bin-links --no-package-lock', {cwd: fixtureDir})


This should be in a different PR.

malept · 2018-04-10T18:40:22Z

test/fixtures/asar-prebuilt/package.json

+{
+  "name": "PrebuiltAsar",
+  "version": "0.0.1",
+  "description": "A test of the renamed from `electron-prebuilt` to `electron`",


The description needs to be updated.

malept · 2018-04-10T18:42:01Z

platform.js

@@ -154,6 +154,19 @@ class App {

    const dest = path.join(this.originalResourcesDir, 'app.asar')
    debug(`Running asar with the options ${JSON.stringify(asarOptions)}`)
+
+    if (asarOptions.filename) {


I think that if there are other asarOptions other than filename and filename is set, it should warn that the other options won't be used.

should that happen here? or in the argument parsing?

It should happen here. If it happens in argument parsing, people who use the JS API (such as users of Electron Forge or ember-electron) will not get the warning.

malept · 2018-04-10T18:43:33Z

platform.js

+          debug(`Copying asar: ${src} to ${target}`)
+          return fs.copy(src, target, {overwrite: false, errorOnExist: true})
+        })
+        .then(() => fs.remove(this.originalResourcesAppDir))


I think for efficiency purposes, if asar.filename is set, we shouldn't even copy the app directory in the first place. Then we can get rid of this fs.remove call.

I started to go down that route, but tried to keep the change minimal. I'll short circuit that.

malept · 2018-04-10T18:45:41Z

platform.js

+
+      return fs.stat(src)
+        .then(stat => {
+          const target = stat.isFile() ? dest : this.originalResourcesDir


Could you explain how target is determined? Shouldn't target always be dest?

if the user specified a directory (containing their app.asar and additional resources) fs-extra's copy requires the dest to be a directory... if the src is a file, it requires dest to be a file.

It's not clear from the docs that you can specify a directory for asar.filename. Although even if it did, I would disagree with it because the name of the option makes that choice not obvious. I think the value of asar.filename should only be an ASAR file.

malept · 2018-04-10T18:48:32Z

test/fixtures/asar-prebuilt/package.json

+  "main": "main.js",
+  "devDependencies": {
+    "electron": "^1.8.0",
+    "electron-prebuilt": "^1.4.0"


There should only be one item in devDependencies, and it should be "electron": "1.3.1". Every time we add a new Electron version to the tests, it takes longer to run the tests due to downloading prebuilt binaries.

malept · 2018-04-10T20:25:00Z

NEWS.md

@@ -4,6 +4,10 @@

 [Unreleased]: https://github.com/electron-userland/electron-packager/compare/v12.0.0...master

+### Added
+
+* `asar.filename` to support pre-built asar files.


Minor nit, remove the period and add (#823) at the end.

malept · 2018-04-10T20:26:24Z

docs/api.md

@@ -183,6 +183,7 @@ is not restricted to the official set if [`download.mirror`](#download) is set.
 *Boolean* or *Object* (default: `false`)

 Whether to package the application's source code into an archive, using [Electron's archive format](https://github.com/electron/asar). Reasons why you may want to enable this feature are described in [an application packaging tutorial in Electron's documentation](http://electron.atom.io/docs/v0.36.0/tutorial/application-packaging/). When the value is `true`, pass default configuration to the `asar` module. The configuration values listed below can be customized when the value is an `Object`. Supported parameters include, but are not limited to:
+- `filename` (*String*): A path to a pre-built asar file. This will circumvent the asar generation and lets you specify your own pre-built asar file.


This is kind of redundant. Let me think about a better way to word this.

malept · 2018-04-10T20:28:25Z

platform.js

-        return true
-      }
-    })
+    const asarOptions = common.createAsarOpts(this.opts) || {}


At this point asarOptions should be an instance variable, no need to calculate it twice.

malept · 2018-04-10T20:29:09Z

platform.js

+      : fs.copy(this.opts.dir, this.originalResourcesAppDir, {
+        filter: ignore.userIgnoreFilter(this.opts),
+        dereference: this.opts.derefSymlinks
+      })


I would rather have a real if statement than a multi-line ternary statement.

malept · 2018-04-10T20:30:45Z

platform.js

+        } else {
+          return true
+        }
+      })


I don't know whether afterPrune and afterCopy hooks should be run, since neither of those operations actually took place, and this.originalResourcesAppDir doesn't actually exist.

Yeah, I didn't know what those hooks were for... I've changed it to just return early.

I don't think returning early is the right solution... we may have to adjust the hook signature so that it's either the path to the resources app directory, or the path to the app.asar file (which would be a breaking change).

@MarshallOfSound since you have dealt with a lot of code relating to the hooks, you might have some insight here.

malept · 2018-04-10T20:56:02Z

test/asar.js

+      return fs.pathExists(path.join(resourcesPath, 'app'))
+    }).then(exists => {
+      return t.false(exists, 'app subdirectory should NOT exist when app.asar is built')
+    })


This can be:

}).then(exists => t.false(exists, 'app subdirectory should NOT exist when app.asar is built'))

malept · 2018-04-10T20:56:49Z

platform.js

+      const src = path.resolve(this.asarOptions.filename)
+      if (Object.keys(this.asarOptions).length > 1) {
+        common.warning('asar.filename has been specified, all other asar options will be ignored')
+      }


This should have a test.

what do you mean? Test that it warns? or test that the other options are ignored? If the later, how should that be done?

Test that it warns. This is an example of doing that (although it should probably be genericized into a function):

https://github.com/electron-userland/electron-packager/blob/a7437b908e68e97b288173759d0538efe7798c82/test/mas.js#L18-L33

malept

I'll need to take another pass at the diff later tonight, but one thing I noticed is that it should also be documented in usage.txt (which I should replace with yargs declarations...).

malept · 2018-04-10T21:44:00Z

I'm also a little worried about the drop in code coverage, but let's see what it looks like after the latest changes go through CI.

jsg2021 · 2018-04-10T21:49:40Z

I updated the usage.txt.

malept · 2018-04-10T21:54:01Z

platform.js

+      return fs.stat(src)
+        .then(stat => {
+          if (!stat.isFile()) {
+            throw new Error(`${src} must be an asar file.`)


This needs a test.

malept

Aside from the suggestions below, the one thing holding back this PR is how to deal with the hooks. I'm pretty sure that omitting the hooks when asar.filename is set will break Electron Forge, but I don't know exactly what the correct way to resolve that is.

malept · 2018-04-11T01:30:23Z

docs/api.md

@@ -183,6 +183,7 @@ is not restricted to the official set if [`download.mirror`](#download) is set.
 *Boolean* or *Object* (default: `false`)

 Whether to package the application's source code into an archive, using [Electron's archive format](https://github.com/electron/asar). Reasons why you may want to enable this feature are described in [an application packaging tutorial in Electron's documentation](http://electron.atom.io/docs/v0.36.0/tutorial/application-packaging/). When the value is `true`, pass default configuration to the `asar` module. The configuration values listed below can be customized when the value is an `Object`. Supported parameters include, but are not limited to:
+- `filename` (*String*): A path to a pre-built asar file. (This will circumvent the asar generation)


A path to a pre-built asar file (this will circumvent asar generation).

malept · 2018-04-11T01:32:23Z

test/asar.js

+  sinon.spy(console, 'warn')
+  return util.packageAndEnsureResourcesPath(t, opts)
+    .then(generatedResourcesPath => {
+      console.warn.calledWithExactly('WARNING: asar.filename has been specified, all other asar options will be ignored')


Shouldn't this be wrapped in t.true()?

sorry, i’ve been in jest land for a while, i assumed sinon made the assertion. i’ll update.

malept · 2018-04-11T01:32:51Z

usage.txt

@@ -25,6 +25,7 @@ asar               whether to package the source code within your app into an ar
                   --asar and its sub-properties simultaneously.

                   Properties supported:
+                   - filename: path to a pre-built asar file. (This will circumvent the asar generation)


path to a pre-built asar file (this will circumvent asar generation)

jsg2021 · 2018-04-11T03:16:49Z

None of this is breaking existing configurations. Without adding the asar.filename option, nothing changes.

The whole point of this option is to take control of the app bundle generation... the hooks can be external to this once the filename is used... if something is needed to be done to the app package, a more general hook can be added?

jsg2021 · 2018-04-11T03:25:30Z

~~the after prune hook still needs to be called since its unrelated to copy.~~

Never mind, I think I understand those hooks now. With a pre-built asar, you don't need the afterCopy/Prune hooks... all processing has already happened when the asar was generated outside of this tool. I can't think of anything you would need to hook for when specifying your own asar... so I'm sticking with my last comment. Just add a new pre-finalize hook called near the end before signing to allow last minute changes...

MarshallOfSound · 2018-04-11T04:08:04Z

RE the hooks issue I believe the operation should throw an error if afterCopy or afterPune hooks are provided and a prebuilt ASAR file is provided , these are incompatible options.

Also I don't think this option should go into the options.asar option. That object is typically sent pretty much through to the ASAR module. This goes nowhere near the ASAR module, I'd prefer this if it was something like options.prebuiltAsar. This should also throw errors if that option is used in conjunction with any other ASAR option.

Also @malept we should just make forge error out instantly if this option is used 😄

malept · 2018-04-11T04:56:41Z

The docs should be updated to note the asar, afterCopy, and afterPrune incompatibilities.

jsg2021 · 2018-04-11T18:07:00Z

I think I got all the suggestions implemented.

malept

I wonder if it would be cleaner to deal with prebuiltAsar this way:

initialize () {
    debug(`Initializing app in ${this.stagingPath} from ${this.templatePath} template`)

    return fs.move(this.templatePath, this.stagingPath, { clobber: true })
      .then(() => {
        if (this.opts.prebuiltAsar) {
          return this.copyPrebuiltAsar()
            .then(() => this.removeDefaultApp())
        } else {
          return this.copyTemplate()
            .then(() => this.removeDefaultApp())
            .then(() => this.asarApp())
        }
      })
}

malept · 2018-04-11T18:12:35Z

common.js

+      warning('--prebuiltAsar and --asar options are incompatible. Ignoring --asar')
+      delete args.asar
+    }
+    console.log('\n\n%s\n\n', JSON.stringify(args, null, 2))


✂️ debug line

malept · 2018-04-11T18:13:28Z

common.js

+      warning('--prebuiltAsar and --ignore are incompatible. Ignoring --ignore')
+    }
+
+    if (!args['deref-symlinks']) {


Should be !args.derefSymlinks

malept · 2018-04-11T18:14:52Z

common.js

+      warning('--prebuiltAsar and --no-deref-symlinks are incompatible. Ignoring --no-deref-symlinks')
+    }
+  }
+


This should be outside of the CLI argument parsing, as I mentioned yesterday, because it will not give warnings to consumers of the JavaScript API.

malept · 2018-04-11T18:15:22Z

docs/api.md

@@ -67,6 +67,10 @@ packager({
 })
 ```

+**NOTE:** `afterCopy` will not be called if `prebuiltAsar` is set.
+
+---


The lines are unnecessary.

I thought it was a nice visual grouping. I'll drop them.

I think if we're going to do that, it should be in a separate PR.

malept · 2018-04-11T18:17:05Z

docs/api.md

+
+A path to a pre-built asar file. 
+
+**NOTE:** This option circumvents many options such as `asar`, `afterCopy`, `afterPrune`, `ignore`, `prune`, or any option related to the copy processes. These options are for generating an app directory/asar with this tool. However, if a prebuilt asar file is provided, these options are not needed.


I'd get rid of , or any option related to the copy processes. Also, it's a little too verbose in general, but I need some time to think of a better way to word it.

malept · 2018-04-11T18:17:31Z

platform.js

-    if ((this.asarOptions || {}).filename) {
+    if (this.opts.prebuiltAsar) {
+      if (this.opts.afterCopy) {
+        return Promise.reject(new Error('afterCopy is incompatible with prebuiltAsar'))


You should just be able to throw new Error.

This isn't an async function, I was trying to make as little of a splash as possible. I hadn't looked to see if the callers were setup to handle thrown exceptions and not just promise rejections. I'll make it throw instead.

jsg2021 · 2018-04-11T20:24:48Z

I've updated to use your suggested initialize()

jsg2021 · 2018-04-11T21:10:45Z

looks like a transient error caused the signing test to fail for one of the jobs... but the rest seem to be passing. I haven't looked further into it, since its outside the scope of my changes and it has been passing.

jsg2021 · 2018-04-20T04:58:11Z

Anything I should change?

jsg2021 · 2018-04-25T21:50:24Z

@malept Would you like anything else? or do you think this is good?

jsg2021 · 2018-05-03T16:23:14Z

I rebased from your current master

MarshallOfSound · 2018-05-04T02:07:13Z

NEWS.md

@@ -24,6 +24,10 @@

 * Upgraded `galactus` to `^0.2.1` to fix a bug with relative paths

+### Added


This is now in the wrong place

ah, should be fixed now

jsg2021 · 2018-05-11T05:56:09Z

I hope all is well? Is this still wanted?

jsg2021 · 2018-05-16T20:23:55Z

I rebased again, let me know if any docs need touch ups.

jsg2021 · 2018-05-19T05:22:03Z

@malpet is there reservations for merging this?

jsg2021 · 2018-05-29T19:18:27Z

Your cleanup looks way better than my changes. :) I didn't want to change too much without fully comprehending the codebase. Sorry if I misunderstood your requests. Hope this gets merged 👍

jsg2021 · 2018-07-02T20:17:49Z

I've been using this branch to bundle my apps, the flag works well. Hope we can merge soon.

jsg2021 · 2018-08-04T19:58:55Z

Is this blocked by something? Anything I could help with?

Fixes #161.

welcome · 2018-10-25T04:36:12Z

Thanks for your contribution! 🎉

jsg2021 · 2018-10-25T14:20:35Z

🎉

malept self-requested a review April 10, 2018 18:34

malept changed the title ~~Implement #161 adds --asar.filename support~~ Add support for asar.filename Apr 10, 2018

malept requested changes Apr 10, 2018

View reviewed changes

malept reviewed Apr 10, 2018

View reviewed changes

malept requested changes Apr 11, 2018

View reviewed changes

malept changed the title ~~Add support for asar.filename~~ Add support for prebuilt asars May 3, 2018

MarshallOfSound reviewed May 4, 2018

View reviewed changes

jsg2021 and others added 2 commits October 2, 2018 19:13

Add prebuiltAsar option

a30d2cc

Fixes #161.

Clean up

2183e42

malept approved these changes Oct 25, 2018

View reviewed changes

malept merged commit b487df2 into electron:master Oct 25, 2018

jsg2021 deleted the asar-filename branch October 25, 2018 14:20

		@@ -4,6 +4,8 @@

		[Unreleased]: https://github.com/electron-userland/electron-packager/compare/v12.0.0...master

		* Added `asar.filename` to support pre-built asar files.


		A path to a pre-built asar file.

		NOTE: This option circumvents many options such as `asar`, `afterCopy`, `afterPrune`, `ignore`, `prune`, or any option related to the copy processes. These options are for generating an app directory/asar with this tool. However, if a prebuilt asar file is provided, these options are not needed.

		@@ -24,6 +24,10 @@

		* Upgraded `galactus` to `^0.2.1` to fix a bug with relative paths

		### Added

Add support for prebuilt asars #823

Add support for prebuilt asars #823

Conversation

jsg2021 commented Apr 10, 2018

welcome bot commented Apr 10, 2018

codecov bot commented Apr 10, 2018 • edited

Codecov Report

malept left a comment

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsg2021 Apr 10, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept left a comment

Choose a reason for hiding this comment

malept commented Apr 10, 2018

jsg2021 commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept left a comment

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

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

jsg2021 commented Apr 11, 2018

jsg2021 commented Apr 11, 2018 • edited

MarshallOfSound commented Apr 11, 2018

malept commented Apr 11, 2018

jsg2021 commented Apr 11, 2018

malept left a comment • edited

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

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

Choose a reason for hiding this comment

jsg2021 Apr 11, 2018 • edited

Choose a reason for hiding this comment

jsg2021 commented Apr 11, 2018

jsg2021 commented Apr 11, 2018

jsg2021 commented Apr 20, 2018

jsg2021 commented Apr 25, 2018

jsg2021 commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsg2021 commented May 11, 2018

jsg2021 commented May 16, 2018

codecov bot commented Apr 10, 2018 •

edited

jsg2021 Apr 10, 2018 •

edited

jsg2021 commented Apr 11, 2018 •

edited

malept left a comment •

edited

jsg2021 Apr 11, 2018 •

edited