Page MenuHomePhabricator

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

Authored by varun on Aug 6 2024, 6:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 3:16 AM
Unknown Object (File)
Sat, Nov 2, 3:16 AM
Unknown Object (File)
Sat, Nov 2, 3:15 AM
Unknown Object (File)
Sat, Nov 2, 3:00 AM
Unknown Object (File)
Fri, Oct 25, 6:49 AM
Unknown Object (File)
Tue, Oct 22, 6:29 PM
Unknown Object (File)
Fri, Oct 18, 5:16 PM
Unknown Object (File)
Fri, Oct 18, 5:16 PM
Subscribers

Details

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 requested review of this revision.Aug 6 2024, 6:33 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

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

yarn.lock
17858

D13012 is this the right way to update all of these to 2.20.0?