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

fix(maker): allow most appx default config to be overridden by the user #121

Merged
merged 4 commits into from
Mar 8, 2017

Conversation

malept
Copy link
Member

@malept malept commented Jan 31, 2017

ISSUES CLOSED: #119

Have you read the section in CONTRIBUTING.md about pull requests?

Yes

Are your changes appropriately documented?

Yes (arguably the docs did not match the code)

Do your changes have sufficient test coverage?

Yes

Does the testsuite pass successfully on your local machine?

Yes

Summarize your changes:

Allow users to override most of the default electron-windows-store config set by Forge.

Fixes #119.

@felixrieseberg are there any of the default params aside from inputDirectory and outputDirectory that we do not want the user to override? FOr instance, perhaps packageExecutable?

@malept malept added the bug label Jan 31, 2017
@felixrieseberg
Copy link
Member

Nah, I'd allow them to override pretty much anything.

@malept
Copy link
Member Author

malept commented Jan 31, 2017

Ugh, need to fix test fixtures before merging. I'll do that when I get home (if no one else beats me to it).

@malept malept force-pushed the appx-params-order branch 2 times, most recently from 85c5bd4 to 6b493ae Compare February 1, 2017 05:11
@malept
Copy link
Member Author

malept commented Feb 1, 2017

OK, I've expended enough brain cells trying to fix this tonight. Maybe someone else wants a go? 😄

@@ -152,6 +152,12 @@ describe(`electron-forge API (with installer=${installer.substr(12)})`, () => {
dir = path.resolve(os.tmpdir(), `electron-forge-test-${dirID}/electron-forge-test`);
dirID += 1;
await forge.init({ dir });

const packageJSON = await readPackageJSON(dir);
packageJSON.name = 'testApp';
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid app name (could be causing the issues) has be all lower case

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the error in question:

MakeAppx : error: Error info: error C00CE169: App manifest validation error: The app manifest must be valid as per schema: Line 6, Column 13, Reason: '' violates minLength constraint of '3'.
The attribute 'Name' with value '' failed to parse.
MakeAppx : error: Package creation failed.
MakeAppx : error: 0x80080204 - The specified package format is not valid: The package manifest is not valid.

How I read that was that it failed because the app name was blank for some reason. But I haven't actually used appx packaging before 😰

@malept malept force-pushed the appx-params-order branch 4 times, most recently from 626762b to 51b3a5b Compare February 4, 2017 08:18
@malept
Copy link
Member Author

malept commented Feb 4, 2017

I am very confused as to why I'm getting errors for Squirrel now: https://ci.appveyor.com/project/electron-userland/electron-forge/build/1.0.266#L7942

@MarshallOfSound
Copy link
Member

@malept Looking into it now on my windows machine 👍

@MarshallOfSound
Copy link
Member

@malept I fixed this locally, but now I can't remember what I did to fix it.... Will check my laptop tonight 😆

@malept malept force-pushed the appx-params-order branch from 0c053bb to 78dae56 Compare March 6, 2017 00:54
Working around bug in electron-installer-redhat
(see: electron-userland/electron-installer-redhat#45).
@malept malept force-pushed the appx-params-order branch from 78dae56 to 5484c2b Compare March 6, 2017 01:09
malept referenced this pull request Mar 7, 2017
This PR adds a new make type for Windows, "appx", which generates Windows Store packages

ISSUES CLOSED: #27
@malept
Copy link
Member Author

malept commented Mar 7, 2017

@MarshallOfSound when you have time, please go grab that fix 😄

@MarshallOfSound
Copy link
Member

Jesus I keep not doing this... 😆

@malept
Copy link
Member Author

malept commented Mar 8, 2017

Yay, tests finally pass 🎉 🙌

@malept malept merged commit 84031ec into master Mar 8, 2017
@malept malept deleted the appx-params-order branch March 8, 2017 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants