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.

Sign up for GitHub

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

Jump to bottom

Fix macos-notification-state build issue #2310

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Fix macos-notification-state build issue #2310

merged 2 commits into from
Oct 24, 2022

Conversation

devinbinnie
Copy link
Member

Summary

There's been a long running issue with the macos-notification-state node module wouldn't compile correctly under npm run postinstall, due to it missing an install script.

This PR adds that bit to the module as part of a postinstall script, and switches us over to using electron-builder to run our app dependencies.

NONE

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 21, 2022
@devinbinnie devinbinnie added this to the v5.2.0 milestone Oct 21, 2022
@devinbinnie devinbinnie requested a review from tboulis October 21, 2022 20:25
@devinbinnie
Copy link
Member Author

cc @amyblais This is a build process fix I'd like to include in v5.2, low risk.

@devinbinnie devinbinnie added the Build Apps for PR Builds signed builds for testing label Oct 21, 2022
@mattermod mattermod removed the Build Apps for PR Builds signed builds for testing label Oct 21, 2022
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod
Copy link
Contributor

Successfully building:
https://circleci.com/gh/mattermost/desktop/23932
https://circleci.com/gh/mattermost/desktop/23931
https://circleci.com/gh/mattermost/desktop/23930

@tboulis
Copy link
Contributor

tboulis commented Oct 24, 2022

Seems like we need to do this only for macos, the windows build fails

@devinbinnie
Copy link
Member Author

@tboulis The good ol' node-gyp fails on Windows error is back. It could just be the Node version causing this. I'm gonna mess with it a bit and see if I can get it working. This always happens in CI...

@tboulis
Copy link
Contributor

tboulis commented Oct 24, 2022

@devinbinnie Do we actually need to build macos-notification-state on Windows? 🤔

@devinbinnie
Copy link
Member Author

@devinbinnie Do we actually need to build macos-notification-state on Windows? 🤔

If I can find a way around it, no. But that's not the issue actually, it's the windows-focus-assist module that's causing the error, or rather just native code building in general. It always throws this weird error where node-gyp can't find the node header files, I still don't exactly know why it happens. We fixed it before by making sure they were pre-downloaded, but that's not working correctly right now. Will have to investigate.

@mattermod
Copy link
Contributor

The file .circleci/config.yml is in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@devinbinnie devinbinnie requested a review from tboulis October 24, 2022 14:18
@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 24, 2022
@devinbinnie devinbinnie merged commit 83b6c64 into mattermost:master Oct 24, 2022
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermost-build mattermost-build mentioned this pull request Oct 24, 2022
Merged
mattermost-build pushed a commit to mattermost-build/desktop that referenced this pull request Oct 24, 2022
@devinbinnie @mattermost-build
Fix macos-notification-state build issue (mattermost#2310)
6cb320c 
* Fix macos-notification-state build issue

* Fix windows build, a bit of cleanup

(cherry picked from commit 83b6c64)
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 24, 2022
devinbinnie added a commit that referenced this pull request Oct 24, 2022
@mattermost-build @devinbinnie
Fix macos-notification-state build issue (#2310) (#2313)
27a64cc 
* Fix macos-notification-state build issue

* Fix windows build, a bit of cleanup

(cherry picked from commit 83b6c64)

Co-authored-by: Devin Binnie <52460000+devinbinnie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tboulis tboulis tboulis approved these changes

Assignees

@devinbinnie devinbinnie

Labels
4: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone release-note-none
Projects
None yet
Milestone
v5.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@devinbinnie @mattermod @tboulis @mm-cloud-bot