Infer --app-version from package.json #449

zeke · 2016-08-11T23:21:28Z

This infers the --app-version from the package.json version property if it has not been passed in through the command line.

For an app using Electron 1.2.6 with a package.json version of 1.1.0:

Before	After

cc pairing with @kevinsawicki @jlord

Fixes #306

malept · 2016-08-11T23:23:09Z

.gitignore

@@ -6,3 +6,4 @@ node_modules
 test/work
 .DS_Store
 .nyc_output
+basicTest-*/**/*


shouldn't that be ignored in test/work?

malept · 2016-08-11T23:41:40Z

NEWS.md

@@ -2,6 +2,10 @@

 ## Unreleased

+### Added
+
+* The `package.json` `version` property is the default app version if `--app-version` is unspecified.


Add GitHub PR # so you get credit 😄

malept · 2016-08-12T01:46:58Z

I added the docs-needed tag because I think this needs to be mentioned in at least docs/api.md. Not sure if it needs to be mentioned in the readme.

jlord · 2016-08-12T04:35:09Z

@malept Oh yeah, of course! Our oversight. I'll add that 👍

malept · 2016-08-12T14:10:26Z

The consistent CI failures on OSX remind me why I didn't implement this feature myself - I couldn't figure out a way to get Wine to cooperate on Travis CI's OSX workers, so I just disabled those tests. Might have to do the same thing here.

malept · 2016-08-12T22:46:44Z

test/ci/before_install.sh

@@ -24,5 +24,6 @@ case "$TRAVIS_OS_NAME" in
      -in codesign.csr -out codesign.cer
    openssl pkcs12 -export -in codesign.cer -inkey codesign.key -out codesign.p12 -password pass:12345
    security import codesign.p12 -k ~/Library/Keychains/login.keychain -P 12345 -T /usr/bin/codesign
+    brew install wine


FYI, the last time I tried this I had to do some weird things so that the OSX CI builds didn't take forever to run and used a relatively recent version of Wine: https://github.com/electron-userland/electron-packager/blob/v7.1.0/test/ci/before_install.sh#L6-L11

Why did you end up taking the brew installation out?

Tests involving rcedit kept timing out.

malept · 2016-08-13T03:38:20Z

I think I'm going to drop this for the 7.6.0 release and save it for a future non-patch release (which will cause a merge conflict in NEWS.md).

malept · 2016-08-16T22:00:56Z

test/ci/before_install.sh

@@ -24,5 +24,6 @@ case "$TRAVIS_OS_NAME" in
      -in codesign.csr -out codesign.cer
    openssl pkcs12 -export -in codesign.cer -inkey codesign.key -out codesign.p12 -password pass:12345
    security import codesign.p12 -k ~/Library/Keychains/login.keychain -P 12345 -T /usr/bin/codesign
+    npm install wine-darwin@1.9.16


NPM can install everything, I guess...

malept · 2016-08-17T01:34:44Z

test/util.js

-      t.end()
+
+      if (process.platform === 'darwin' && process.env.TRAVIS) {
+        fs.remove(path.join(process.env.HOME, '.wine'), function () {


This seems like it would make all of the tests that don't specify app-version explicitly slower.

Yeah, this was just an attempt to see what could be causing it to fail on CI, will revert after investigating more.

kevinsawicki · 2016-08-17T18:36:17Z

Got specs passing on Travis CI using wine, https://travis-ci.org/electron-userland/electron-packager/builds/153046665

malept · 2016-08-17T18:39:39Z

Nice! The question becomes, is it consistent, or will tests time out sometimes? The inconsistent timeouts were why I had to stop installing wine on Travis OSX in the first place.

kevinsawicki · 2016-08-17T18:44:47Z

The question becomes, is it consistent, or will tests time out sometimes?

Yeah, since this PR causes way more tests to call rcedit via wine and the build passed on all 3 macs in the matrix configuration, I'm optimistic that things will be consistent given the new way wine is being installed and configured.

I haven't seen any failures on https://travis-ci.org/kevinsawicki/wine-darwin once the WINEDLLOVERRIDES env var was set which is also set now in this PR.

malept · 2016-08-18T01:00:36Z

Not that I want to drag this PR out any longer, but should we have a test for the case where no app-version is specified and (for some reason) there is no version in any of the package.json files found?

malept · 2016-08-18T21:40:11Z

I asked:

[...] should we have a test [...]

We talked about it this morning - I believe where we ended up is that it would be nice but it would take a lot of effort to handle this case, because you'd have to either:

Copy a fixture to a temporary directory (so that the electron-packager package.json doesn't get detected by get-package-info) - and even then, there could be inconsistent failures on dev machines where someone puts a package.json in their root temporary directory
Refactor that code to stop at the first package.json - I'm generally in favor of this method, but the concern was that it would break existing code (even though that wasn't an explicit feature)

@kevinsawicki please correct me if I'm misremembering.

My inclination is for the second option to happen, but not in this PR. It would also obviously be a breaking change that requires a major version bump.

zeke · 2016-08-19T17:40:13Z

🎉

malept · 2016-08-19T18:28:23Z

Feel free to release 7.7.0 (or I can do it sometime after this afternoon). I need to add a couple of steps to the release docs for after running git push:

Create a new GitHub release from the pushed tag with the contents of NEWS for that version
Close the milestone associated with the version if one is open

This was removed in https://github.com/desktop/desktop/pull/307/files#r76084401 because we thought `build-version` was inferred by `electron-packager`. But it turns out it only infers `app-version`: electron/packager#449

malept reviewed Aug 11, 2016
View reviewed changes

.gitignore

@@ -6,3 +6,4 @@ node_modules

test/work

.DS_Store

.nyc_output

basicTest-*/**/*

Copy link

Member

malept Aug 11, 2016

Choose a reason for hiding this comment

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

shouldn't that be ignored in test/work?

zeke force-pushed the default-version branch from 7b3e88a to 89b57d9 Compare August 11, 2016 23:26

malept reviewed Aug 11, 2016
View reviewed changes

malept added the docs-needed 📝 Pull request needs documentation label Aug 11, 2016

malept modified the milestone: 7.6.0 Aug 12, 2016

malept added needs work 🚧 Feature request needs to be fleshed out before maintainers can take action on it and removed docs-needed 📝 Pull request needs documentation labels Aug 12, 2016

kevinsawicki force-pushed the default-version branch from 64b6a4e to ccb61b1 Compare August 12, 2016 20:26

zeke and others added 6 commits August 12, 2016 14:35

Default app/build version to package.json version

f1ecd4d

Keep dependency props at end of array

cb33caa

Remove unneeded build version check

1724a75

Add the good news

a302279

Add PR number to news

db793fc

Update docs

a8a9f69

kevinsawicki force-pushed the default-version branch from ccb61b1 to a8a9f69 Compare August 12, 2016 21:35

brew install wine on travis macs

d301bc8

malept reviewed Aug 12, 2016
View reviewed changes

malept modified the milestone: 7.6.0 Aug 13, 2016

jlord added 2 commits August 16, 2016 14:51

Use wine package from npm

4b697d8

Merge branch 'master' into default-version

d97c3b9

malept reviewed Aug 16, 2016
View reviewed changes

Upgrade wine-darwin version

685a8fe

malept reviewed Aug 17, 2016
View reviewed changes

kevinsawicki added 2 commits August 17, 2016 10:14

git ignore npm-debug.log

f52d168

Upgrade and configure wine on Travis

dccc008

kevinsawicki force-pushed the default-version branch from 601de7f to dccc008 Compare August 17, 2016 17:47

Use relative path to wine

998de30

malept removed the needs work 🚧 Feature request needs to be fleshed out before maintainers can take action on it label Aug 17, 2016

malept added this to the 7.7.0 milestone Aug 17, 2016

Suppress Wine fixme: log messages

a5633e0

malept merged commit 7abbe62 into master Aug 19, 2016

malept deleted the default-version branch August 19, 2016 15:03

zeke mentioned this pull request Aug 20, 2016

Simplify package.json inference #461

Closed

joshaber mentioned this pull request Sep 23, 2016

Explicitly set build-version desktop/desktop#387

Merged

This was referenced Sep 27, 2016

Build version should default to app version on Windows #498

Closed

Default build version to app version on Windows #501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer --app-version from package.json #449

Infer --app-version from package.json #449

zeke commented Aug 11, 2016 •

edited

Loading

malept Aug 11, 2016

malept Aug 11, 2016

malept commented Aug 12, 2016

jlord commented Aug 12, 2016

malept commented Aug 12, 2016

malept Aug 12, 2016

zeke Aug 12, 2016

malept Aug 12, 2016

malept commented Aug 13, 2016

malept Aug 16, 2016

malept Aug 17, 2016 •

edited

Loading

kevinsawicki Aug 17, 2016

kevinsawicki commented Aug 17, 2016

malept commented Aug 17, 2016

kevinsawicki commented Aug 17, 2016

malept commented Aug 18, 2016

malept commented Aug 18, 2016

zeke commented Aug 19, 2016

malept commented Aug 19, 2016

Infer --app-version from package.json #449

Infer --app-version from package.json #449

Conversation

zeke commented Aug 11, 2016 • edited Loading

malept Aug 11, 2016

Choose a reason for hiding this comment

malept Aug 11, 2016

Choose a reason for hiding this comment

malept commented Aug 12, 2016

jlord commented Aug 12, 2016

malept commented Aug 12, 2016

malept Aug 12, 2016

Choose a reason for hiding this comment

zeke Aug 12, 2016

Choose a reason for hiding this comment

malept Aug 12, 2016

Choose a reason for hiding this comment

malept commented Aug 13, 2016

malept Aug 16, 2016

Choose a reason for hiding this comment

malept Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

kevinsawicki Aug 17, 2016

Choose a reason for hiding this comment

kevinsawicki commented Aug 17, 2016

malept commented Aug 17, 2016

kevinsawicki commented Aug 17, 2016

malept commented Aug 18, 2016

malept commented Aug 18, 2016

zeke commented Aug 19, 2016

malept commented Aug 19, 2016

zeke commented Aug 11, 2016 •

edited

Loading

malept Aug 17, 2016 •

edited

Loading