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

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

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

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

docs/api.md

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

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

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

test/_setup.js

This should be in a different PR.

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

test/fixtures/asar-prebuilt/package.json

The description needs to be updated.

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

platform.js

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

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

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

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

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

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

docs/api.md

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

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

platform.js

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

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

platform.js

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

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

platform.js

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

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

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

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

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

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

test/asar.js

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

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

✂️ debug line

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

common.js

Should be !args.derefSymlinks

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

common.js

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

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

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

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

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

Uh oh!

Add support for prebuilt asars #823

Add support for prebuilt asars #823

Uh oh!

Conversation

jsg2021 commented Apr 10, 2018

Uh oh!

welcome bot commented Apr 10, 2018

Uh oh!

codecov bot commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

malept left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsg2021 Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malept left a comment

Choose a reason for hiding this comment

Uh oh!

malept commented Apr 10, 2018

Uh oh!

jsg2021 commented Apr 10, 2018

codecov bot commented Apr 10, 2018 •

edited

Loading

jsg2021 Apr 10, 2018 •

edited

Loading

jsg2021 commented Apr 11, 2018 •

edited

Loading