[windows] override C++20 and upgrade nan package
ClosedPublic
Actions

Authored by varun on Aug 6 2024, 6:16 PM.

Details

Reviewers

ashoat
marcin

Commits

rCOMM1e1c695ca3dc: [windows] override C++20 and upgrade nan package

Summary

We specify C++17 to override the default C++ edition for Visual Studio, C++20, which is not compatible with ZW. Additionally, the addon needs a newer version of the nan package to compile.

Test Plan

able to build the windows push notifications addon now

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Aug 6 2024, 6:16 PM

Herald added a subscriber: tomek. · View Herald TranscriptAug 6 2024, 6:16 PM

Harbormaster completed remote builds in B30966: Diff 43195.Aug 6 2024, 6:33 PM

varun requested review of this revision.Aug 6 2024, 6:33 PM

ashoat accepted this revision.Aug 6 2024, 6:37 PM

ashoat added inline comments.

desktop/addons/windows-pushnotifications/binding.gyp
26 ↗	(On Diff #43195)	Did we confirm this was necessary after we upgraded `nan`? Want to make sure it’s necessary… would rather have fewer config options if they’re not necessary

This revision is now accepted and ready to land.Aug 6 2024, 6:37 PM

realized i forgot to add the lockfile. will update before landing

desktop/addons/windows-pushnotifications/binding.gyp
26 ↗	(On Diff #43195)	we need both. the build fails with the "incompatibility between c++20 and ZW" error otherwise

Closed by commit rCOMM1e1c695ca3dc: [windows] override C++20 and upgrade nan package (authored by varun). · Explain WhyAug 6 2024, 9:30 PM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMM1e1c695ca3dc: [windows] override C++20 and upgrade nan package.

ashoat added inline comments.Aug 7 2024, 9:51 AM

yarn.lock
17858	Looks like these dependencies are for `fsevents` and `macos-alias`, which are still using the old `nan`: `macos-alias` is used by `@electron-forge/maker-dmg`, which is used in our electron-forge config Version 1 of `fsevents` requires `nan`, but version 2 doesn't. We have dependencies on version 1 due to an old version of Jest that is required by `hazel-server` (we have the latest version of `hazel-server`) Don't think either of those will be affected by the Windows issues we've seen, so we can leave it. But in the future, one thing we could do here would've been to update all of these to 2.20.0, since it looks like it just fixes bugs and shouldn't have negative effects