cordova-electron icon indicating copy to clipboard operation
cordova-electron copied to clipboard

Invalid mas build?

Open buu700 opened this issue 4 years ago • 12 comments

I'm running a version of cordova-electron forked from 1.1.1, with the only change being that Electron is updated to 6.1.7 as you can see here. As far as I can tell based on the output below, this works as intended.

However, my package is being rejected by the Mac App Store: https://github.com/electron/electron/issues/20027#issuecomment-585545947. The replies from the Electron maintainers suggest that this is likely an issue with cordova-electron:

dirty build with dmg in it . . . I'm saying you didn't use the mas build. Whatever tooling you are using is not using the mas build of Electron as that symbol list is basically a history lesson in all the symbols we've removed in the past.

Any ideas?


[Cordova Electron] The built package size may be larger than necessary. Please run with --verbose for more details.
  • electron-builder  version=22.3.2 os=19.2.0
  • writing effective config  file=platforms/electron/build/builder-effective-config.yaml
  • installing production dependencies  platform=darwin arch=x64 appDir=/Users/buu700/cyph/cyph-phonegap-build/platforms/electron/www
  • packaging       platform=darwin arch=x64 electron=6.1.7 appOutDir=platforms/electron/build/mac
  • packaging       platform=mas arch=x64 electron=6.1.7 appOutDir=platforms/electron/build/mas
  • building        target=DMG arch=x64 file=platforms/electron/build/Cyph-1.0.42.dmg
  • signing         file=platforms/electron/build/mas/Cyph.app identityName=3rd Party Mac Developer Application: Cyph, Inc. (SXZZ8WLPV2) identityHash=C8AE49EA521129CB8F6E7EFC648A04CC9385C411 provisioningProfile=/Users/buu700/.cyph/nativereleasesigning/apple/macOS_Distribution.provisionprofile
  • building block map  blockMapFile=platforms/electron/build/Cyph-1.0.42.dmg.blockmap

buu700 avatar Feb 18 '20 03:02 buu700

The Electron and builder dependencies were already updated in the master branch. The master branch is in the process of being prepared for a major release.

There is currently no ETA on the release date.

Releasing a major takes time to prepare as there are many other repos (core tools) that need to be prepared and released before we can release platforms.

As for the commit on your forked branch where you are adding mas as a default target package type, we would like to keep the default target to dmg. But, You can config the build to include mas. Please take a look at our docs first as it talks about how to change/add other target package types.

https://cordova.apache.org/docs/en/latest/guide/platforms/electron/index.html

Sorry that the major release process is taking a long time, as there are a lot of moving parts.

erisu avatar Feb 18 '20 10:02 erisu

Thanks for the quick reply @erisu. Yes, I was previously using the nightly version, but I had to do this in order to get Electron version 6.1.7, as only the most recent 6.x releases (and not 7.x or above) currently include the patch to work in the Mac App Store.

I should mention that I also tested forking master to use Electron 6.1.7, with the same result.

Sorry for the confusion with the commit you mentioned; I should have explained that. Yes, I'm already using the mas build (https://github.com/cyph/app/blob/master/build.json). That was an unsuccessful test for @JCBsystem and @MarshallOfSound's suggestion that the error I'm seeing was due to cordova-electron somehow incorrectly configuring the mas build, rather than an issue in Electron itself.

buu700 avatar Feb 18 '20 12:02 buu700

I was able to build a mas target in the past when I tested.

That most likely would have been around the time we added the information to the docs. I wouldn't say we configured it wrong but I also can not remember if there was changes around the logic that did the translation from build.json to electron-builder's configuration format.

Most likely there might not have been changes.

There are also possibilities that it could have broken naturally over time. For example, Apple had began enforcing notarizing build. With this, some known issues appeared. The next major will also have the notarizing fix.

When I get some free time, I can try to build for the mas target, with current master (nightly) and current release.

erisu avatar Feb 18 '20 13:02 erisu

Just to clarify, if this part wasn't clear, building and signing the mas target appears to work; the issue is that it's being rejected from the Mac App Store, which I'm told by the Electron team is because there's something wrong with the generated .pkg file.

Specifically @MarshallOfSound says that the .pkg file isn't actually a mas build, although I'm still unclear on what else it would be. Does that sound plausible? Or maybe cordova-electron is just incorrectly generating a debug build even when --release is specified?

buu700 avatar Feb 18 '20 13:02 buu700

The log output your showing says it is using the 3rd Party Mac Developer Application for signing. That seems correct as it should be the Mac App Distribution..

• signing file=platforms/electron/build/mas/Cyph.app identityName=3rd Party Mac Developer Application: Cyph, Inc. (SXZZ8WLPV2) identityHash=C8AE49EA521129CB8F6E7EFC648A04CC9385C411 provisioningProfile=/Users/buu700/.cyph/nativereleasesigning/apple/macOS_Distribution.provisionprofile

When you asked on Electron/Electron-Builder GH, did you show them your platforms/electron/build/builder-effective-config.yaml file? That would be their config file.

erisu avatar Feb 18 '20 14:02 erisu

That's good to know, thanks! I didn't, but I'll find and post that in the other thread today.

buu700 avatar Feb 18 '20 14:02 buu700

Okay, we narrowed things down a bit over in the Electron issue thread. Turns out that the electron npm package being included in app.asar.unpacked is the problem. Is cordova-electron doing anything that might cause that?

buu700 avatar Feb 20 '20 09:02 buu700

I would recommend ensuring that all dev related dependencies from package.json are in the devDependencies and not dependencies.

Since Cordova is wrapped around the Electron project, the contents of the dependencies are copied into the app's package.json. This is so that dependencies are bundled with the app but also available for running the electron project with the cordova run electron command.

This is probably why you are getting electron in your built app.

From looking at your package.json you have

	"dependencies": {
		"@cyph/prettier": "*",
		"cordova": "*",
		"cordova-support-google-services": "https://github.com/LuisEGR/cordova-support-google-services.git",
		"electron-push-receiver": "*"
	},

I am not 100% sure if the electron-push-reciever is used in app code or some build process, but the top three IMO should be devDependencies.

This is my first recommendation.

Next would be, after you run npx cordova platform add ${platform} and npx cordova plugin add from your build.sh script, have the script update the package.json by moving them from the dependencies field to devDependencies.

Again platform and plugins are also dev. When they are installed and prepared during build, the prepare steps is what moves contents from these packages to the appropriate location for building. The entire package itself is not meant to be. Since the cordova-electron platform was being bundled with your app, electron was being installed since it is a dependency of cordova-electron.

Also when you make these changes, your final built app should even be smaller in size as all the dev related dependencies and Cordova build scripts are not packaged with the app.

In the current Cordova CLI, all platform and plugins are added as dependencies which is not the correct pattern and in the next major release, this will be fixed.

I think this is what you are seeing.

erisu avatar Feb 20 '20 10:02 erisu

Thanks for explaining all that! I'd been meaning to look into the [Cordova Electron] The built package size may be larger than necessary. warning at some point. So basically anything that I don't explicitly need to require at run-time should be in devDependencies? If that's the case, yep, only electron-push-receiver needs to be in there.

So I just went ahead and implemented your suggestion: https://github.com/cyph/app/commit/ca1170efc77c2cba69d6037a9124a4a685f3e86f

I've verified that this works, but it seems that cordova build electron --release partially undoes it and moves cordova-android, cordova-electron, and cordova-ios all right back into dependencies. electron is still in app.asar.unpacked/node_modules.

I'm also curious about the other dependencies like electron-push-receiver not being in app.asar.unpacked/node_modules; that makes me think that maybe all those dependencies would live somewhere else, and that the inclusion of electron in app.asar.unpacked/node_modules is possibly a separate unrelated issue?

buu700 avatar Feb 21 '20 00:02 buu700

I tried building your project, somewhat, to follow along with,

What I noticed is that the plugins and platforms are all defined in config.xml. Cordova has been moving away from defining plugins and platforms in the config.xml. The newer pattern is to define them directly in the package.json.

When you run the build the first time, you will notice messages like:

Plugin 'cordova-clipboard' found in config.xml... Migrating it to package.json

So there is some changes that can be made to improve this area..

But the main issue right now would be that the platforms are being added back as dependencies. As you pointed out it occurring on cordova build. This is because during build, it triggers a prepare step and it sees in your config.xml these lines:

<engine name="android" spec="^8.1.0" />
<engine name="electron" spec="git+https://github.com/buu700/cordova-electron-tmp" />
<engine name="ios" spec="^5.1.0" />

And it also sees that they are not in package.json dependencies field.

Since the build.sh script uses npx cordova platform add, it is not necessary to be in config.xml. You can delete those lines.

I am not exactly sure on your entire build process and why everything is done in that certain way, for example build.sh installs the platforms and plugins instead of committing the package.json to repo that already pertains all that information. All Cordova related items in the package.json will be checked out if they were missing when running cordova prepare or cordova build. Only an additional npm install is needed if there were non-Cordova related dependencies & devDependencies that are required.

erisu avatar Feb 21 '20 01:02 erisu

I quickly made repo to show how I would have handled having plugins and platforms in the package.json and not have them defined in config.xml.

But like I said earlier, I do not know the entire use case or goal with your build process. There could be something I am not seeing.

https://github.com/erisu/cdvSampleProject

erisu avatar Feb 21 '20 02:02 erisu

Didn't mean to take up so much of your time, but thanks for looking into this!

As far as why the dependencies are organized as they are, it's less a case of "this is the best way to do things in a new project" than "if it ain't broke don't fix it". Specifically, since we've had issues with some plugins meant for one platform being enabled on other platforms, until Cordova adds a built-in solution for platform-specific plugins, we're just leaving the custom solution we already have in place that depends on config.xml.

Aside from that, sounds like all I need to do is remove those three lines from config.xml (or at least remove them before cordova build) and all should work as expected?

buu700 avatar Feb 27 '20 01:02 buu700