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

Compile linux for different architectures separately #238

Closed
wants to merge 7 commits into from

Conversation

mythsunwind
Copy link
Contributor

The node module that does the spellcheck needs to be compiled for the architecture it is run. Currently we only compile it for the architecture of the build server, but we need need to compile it for the specific architectures (ia32 and x64) now.

Example: When compiling the linux build on x64 architecture, we currently build the spellcheck module for x64 architecture and package it together with either ia32 electron or x64 electron into deb files for ia32 and x64. The deb package for ia32 contains the ia32 electron but the x64 node module. Electron will potentially throw an error when trying to load the module, because it is not an ia32 module.

The module is build at the time npm install is run, therefor I propose having two separate tasks for each architecture. Maybe there is a better way.

@lipis
Copy link
Contributor

lipis commented Dec 1, 2016

Since it will work on Windows.. move that to dependencies as well.. https://github.com/wireapp/wire-desktop/blob/master/electron/package.json#L28

@@ -21,10 +21,10 @@
"open-graph": "0.2.1",
"raygun": "0.9.0",
"request": "2.76.0",
"winston": "https://github.com/wireapp/winston.git#2.2.0-e"
"winston": "https://github.com/wireapp/winston.git#2.2.0-e",
"spellchecker": "3.3.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

alphabetically :)

@ConorIA
Copy link
Contributor

ConorIA commented Dec 1, 2016

I think that electron-builder has recently fixed the rebuild command, which was incompatible with yarn. I pushed a PR that disabled re-building for the specific arch before packaging with electron-builder (for Linux), but that could probably be revisted now.

@mythsunwind
Copy link
Contributor Author

@ConorIA can you link the PR here? I could not find it on their github. Would be interesting to have a look.

Currently we had to downgrade to use npm install for building versions instead of yarn because of yarnpkg/yarn#2072 and because we cannot re-build for specific architectures with yarn.

@lipis
Copy link
Contributor

lipis commented Dec 3, 2016

Does the spellchecker work for you @mythsunwind on Windows?

@ConorIA
Copy link
Contributor

ConorIA commented Dec 4, 2016

@mythsunwind, as far as I can tell, the commits related to this issue (electron-userland/electron-builder#861) should make rebuild compatible with yarn, but I haven't been able to make it work on my local install.

@lipis, is the spellchecker enabled for Linux? I see that it is installed as a module, but I can't find the functionality in the app.

@ConorIA
Copy link
Contributor

ConorIA commented Dec 4, 2016

@mythsunwind, @lipis, rebuilding is working with the latest yarn (v0.18.0) release. See #248.

@lipis
Copy link
Contributor

lipis commented Dec 4, 2016

Woho!! I'm not entirely sure how to test and what was the problem.. but we could check in depth tomorrow!

As for the spellchecker it should work on Linux.. was working on my end earlier this week!

@mythsunwind
Copy link
Contributor Author

Will close this PR. Replaced by #248

@mythsunwind mythsunwind closed this Dec 6, 2016
@mythsunwind mythsunwind deleted the linux-spellcheck branch December 6, 2016 09:16
@lipis
Copy link
Contributor

lipis commented Dec 6, 2016

Good stuff...!

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