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 default values to arch and platform #464

Merged
merged 1 commit into from
Aug 26, 2016

Conversation

malept
Copy link
Member

@malept malept commented Aug 22, 2016

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

Yes

Summarize your changes:

The defaults are the host's arch and platform values.

Fixes #36.

Are your changes appropriately documented?

Updated NEWS, readme, API, & CLI docs. I probably need to update the readme as well (have not done that yet).

Do your changes have sufficient test coverage?

Changed the defaults test to test the new platform/arch defaults.

Does the testsuite pass successfully on your local machine?

Yes

@malept malept added this to the 8.0.0 milestone Aug 22, 2016
@malept
Copy link
Member Author

malept commented Aug 23, 2016

@kevinsawicki @jlord @zeke any comments/concerns you have on either the concept or the changes would be much appreciated. This one is likely going to be a fairly controversial change.

@kevinsawicki
Copy link
Contributor

any comments/concerns you have on either the concept or the changes would be much appreciated.

My instinct is to keep 32-bit as the default arch instead of the host arch (which is presumably 64-bit since developers commonly use 64-bit machines). When unspecified, I'm assuming the person wants a package that will work on the most machines possible, and 32-bit will work more places than 64-bit.

For platform though, the host platform sounds like a great default that people will expect 👍

@malept
Copy link
Member Author

malept commented Aug 24, 2016

My rationale for making both defaults the same as the host is that it is easier for the developer to test. (I should have put that in the PR summary.)

I believe that Windows 32-bit executables will run on 64-bit machines (maybe? I could be wrong) but in order for Linux 32-bit executables to run, the whole 32-bit glibc/etc stack also needs to be installed (e.g., dpkg --add-architecture i386), unless for some reason Electron is statically linked to musl or similar 😄

@kevinsawicki
Copy link
Contributor

I believe that Windows 32-bit executables will run on 64-bit machines

Yup, that is my understanding as well.

but in order for Linux 32-bit executables to run, the whole 32-bit glibc/etc stack also needs to be installed (e.g., dpkg --add-architecture i386)

Oh, interesting, didn't realize this.

Since it seems like the better default on Linux is the host arch, and their is no 32-bit on macOS, the host arch is the better default.

One question, if I am on 32-bit Linux and Windows, and I request to build for macOS with no arch specified, will it still build a 64-bit version of mac since their is no 32-bit, or would it error about 32-bit mac being unavailable?

@malept
Copy link
Member Author

malept commented Aug 24, 2016

One question, if I am on 32-bit Linux and Windows, and I request to build for macOS with no arch specified, will it still build a 64-bit version of mac since their is no 32-bit, or would it error about 32-bit mac being unavailable?

It will currently error. It would also error if #106 is fixed and you only specify --arch=armv7l on OSX/Windows.

The defaults are the host's arch and platform values.

Fixes #36.
@malept malept changed the title [RFC] Add default values to arch and platform Add default values to arch and platform Aug 25, 2016
@malept malept merged commit 81f5ddc into electron:master Aug 26, 2016
@malept malept deleted the default-platform-and-arch branch August 26, 2016 00:35
@zeke
Copy link
Contributor

zeke commented Aug 26, 2016

Nice! Are you planning to land this in 8.0.0?

@malept
Copy link
Member Author

malept commented Aug 26, 2016

Yup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants