Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the new `electron` package name #435

Merged
merged 1 commit into from Aug 4, 2016

Conversation

Projects
None yet
3 participants
@zeke
Copy link
Member

commented Aug 2, 2016

A rename of electron-prebuilt to electron is in the works: electron-userland/electron-prebuilt#160

Bothelectron-prebuilt and electron will be published in tandem through the end of 2016.

This PR updates electron-packager to add support for users of the electron package, while maintaining existing functionality for electron-prebuilt.

@kevinsawicki @jlord @zcbenz @iolsen

@malept

This comment has been minimized.

Copy link
Member

commented Aug 2, 2016

You could probably minimize the new electron fixture.

@zeke zeke force-pushed the support-renamed-prebuilt branch from 41be860 to cefb2b8 Aug 3, 2016

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2016

I have a fix in place and the test suite is passing for me locally but CI is failing.

I removed get-package-info because I was hitting an issue described right in its readme:

This is an error from resolve that means it was not able to find all the requested props. It would be nice to have a more descriptive error but there's no way to do that until this PR is merged.

I changed that code to just require the package.json file in the given directory, then use lodash.get to go spelunking for deep properties.

I will dig into the CI failures tomorrow.

NEWS.md Outdated
@@ -2,6 +2,12 @@

## Unreleased

## [7.5.0] - 2016-08-03

This comment has been minimized.

Copy link
@malept

malept Aug 3, 2016

Member

If you're adding this, I assume you're going to handle releasing a new version as soon as you merge. Otherwise, I suggest leaving it off and then the release will happen when it happens (mostly so the date is accurate).

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

Also, add a link comparing 7.4.0 to 7.5.0 like the other versions (including 7.4.0 which I just fixed in c4809e1)

NEWS.md Outdated

### Added

* Add support for the new `electron` package name. #435

This comment has been minimized.

Copy link
@malept

malept Aug 3, 2016

Member

Support for the new electron package name (#435)

I'm trying not to be redundant with the header name. (Doesn't always happen, sadly)

This comment has been minimized.

Copy link
@zeke

zeke Aug 3, 2016

Author Member

Cool. Will fix it once I get CI passing.

@zeke zeke changed the title [WIP] Add support for the new `electron` package name Add support for the new `electron` package name Aug 3, 2016

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2016

Okay this is ready for review.

The previous push passed on 5 of 6 CI boxes; hopefully we'll see all green this time. (CI takes about an hour)

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2016

Happy to squash commits if that is still the convention here.

@malept

This comment has been minimized.

Copy link
Member

commented Aug 3, 2016

Happy to squash commits if that is still the convention here.

Yup, see https://github.com/electron-userland/electron-packager/blob/master/CONTRIBUTING.md#filing-pull-requests (last bullet point)

I will try to review later tonight. (Though I just noticed that somehow test coverage decreased with this PR...hopefully it's because CI isn't done yet.)

TRAVIS_OS_NAME=osx ./test/ci/before_install.sh
```

If you're using Linux:

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

If you're using a Debian/Ubuntu-derived distribution of Linux:

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

Also for Linux, that script is specifically for x86_64 architectures (since Travis only provides those machines). Not sure if it will error if you try to run it on an x86 architecture (mostly because I haven't tried it).

NEWS.md Outdated

### Added

* Support the new `electron` package name. #435

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

The convention is to avoid periods at the end of items and put GitHub issue numbers in parentheses.

t.end(err)
})
}
}

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

Seems like another chance to DRY up some code.

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

I still think this can be "merged" with the other test.

@@ -18,6 +18,9 @@ series([
console.log('Running npm install in fixtures/basic...')
exec('npm install', {cwd: util.fixtureSubdir('basic')}, cb)
}, function (cb) {
console.log('Running npm install in fixtures/basic-renamed-to-electron...')
exec('npm install', {cwd: util.fixtureSubdir('basic-renamed-to-electron')}, cb)
}, function (cb) {

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

Since we keep adding these, might as well genericize it.

(Note to self: add optional part about adding npm install to test fixtures docs)

index.js Outdated
debug('Inferring target Electron version from `electron-prebuilt` dependency or devDependency in package.json')
opts.version = pkg.version
return cb(null)
})

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

It feels like this code can be DRY'd with the electron code above.

@malept

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

Hmmm. The tests no longer hit the "unable to find app name/electron version" code? Also, the test failure on Linux/Node 6 is troubling, but I'm not sure why it's happening (I reran the test run on Travis with the same result).

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Thanks for all the feedback, @malept. I pushed up some changes to the docs and DRY'd things a bit.

I feel like the npm install stuff can live without being optimized for now.

As for the CI, the tests definitely seem flaky. I keep seeing 5/6 passed builds, but the failing environment is not consistently the same. Let's see what happens on this go around.

NEWS.md Outdated

### Added

* Support the new `electron` package name #435

This comment has been minimized.

Copy link
@malept

malept Aug 4, 2016

Member

Parentheses around the issue number, please (for consistency)

@malept

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

I have not been thrilled with this project's test runner (or tests, for that matter). Any suggestions/improvements would be greatly appreciated.

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Travis results from three recent pushes (without meaningful code changes):

screen shot 2016-08-04 at 12 56 34 pm

screen shot 2016-08-04 at 12 58 13 pm

screen shot 2016-08-04 at 12 58 19 pm

@zeke zeke force-pushed the support-renamed-prebuilt branch from 6b63678 to 8aa7dfc Aug 4, 2016

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Squashed it all down into a single commit. Pretty sure this is ready to go. Let's defer on improving the test suite for another PR.

@malept

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

I'm 👍 except for that one new infer test, which I'd like to see fixed before merging.

Let's defer on improving the test suite for another PR.

Agreed.

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

except for that one new infer test

What's wrong with it?

@malept

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

It's mostly duplicate code from the other test (pretty much everything except 3 lines).

@zeke zeke force-pushed the support-renamed-prebuilt branch from 61ba66c to c508500 Aug 4, 2016

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Simplified the test in 61ba66c (good call), and re-squashed commits.

@malept malept merged commit 9cae421 into master Aug 4, 2016

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 97.066%
Details

@malept malept deleted the support-renamed-prebuilt branch Aug 4, 2016

@malept

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

@zeke Thanks! Please release 😄

@zeke

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Thanks for helping out, @malept. Landed in 7.5.0

@SimulatedGREG

This comment has been minimized.

Copy link

commented Aug 6, 2016

Hmmm. The tests no longer hit the "unable to find app name/electron version" code? Also, the test failure on Linux/Node 6 is troubling, but I'm not sure why it's happening (I reran the test run on Travis with the same result).

The 7.5.0 update seems to have broken my setup as I keep getting [Error: must specify version] when building. I then tried installing electron-packager@7.4.0 and used same configuration with no problems.
The item that seems to fix this issue would be having to define the version of electron manually that I'd like to package with. Yet as of the docs I shouldn't have to.

If omitted, it will use the version of the nearest local installation of electron or electron-prebuilt, defined in package.json in either dependencies or devDependencies.

Can confirm this happens with both node 5.10.0 and 6.2.0 on macOS. Here is my full error message...

Error: must specify version
    at download (/Users/gregholguin/Desktop/testing123/node_modules/electron-download/index.js:16:27)
    at Array.<anonymous> (/Users/gregholguin/Desktop/testing123/node_modules/electron-packager/index.js:121:7)
    at each (/Users/gregholguin/Desktop/testing123/node_modules/run-series/index.js:17:24)
    at next (/Users/gregholguin/Desktop/testing123/node_modules/rimraf/rimraf.js:74:7)
    at FSReqWrap.CB [as oncomplete] (/Users/gregholguin/Desktop/testing123/node_modules/rimraf/rimraf.js:110:9)

UPDATE: Currently using electron-prebuilt.

@malept

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

@SimulatedGREG I'm in the middle of releasing a fix (see #439 and #440).

@SimulatedGREG

This comment has been minimized.

Copy link

commented Aug 6, 2016

@malept was just noticing. Thanks for the awesome work! Scary how switching from get-package-info to lodash.get created such a problem 😁.

@malept

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

It basically means we need help with a better testsuite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.