search

microsoft / node-native-keymap

Provide OS keyboard layout functionality as a nodejs module
MIT License
136 stars 37 forks source link

add cf and qspectre flags #39

Closed TylerLeonhardt closed 2 years ago

deepak1556 commented 2 years ago

Windows CI requires change similar to https://github.com/microsoft/vscode/blob/a05babfa55a132b26d97019e9936cf0501a5fd19/.github/workflows/ci.yml#L61-L66 to use 2022 toolchain from windows-latest agent.

alexdima commented 2 years ago

@TylerLeonhardt @deepak1556 I'm a bit out of the loop here. This native node module is one of many native node modules that we ship with VS Code. Should these flags be added to the yarn invocation (in the VS Code build) to cover all native node modules, not just those that are owned by us?

deepak1556 commented 2 years ago

@alexdima we could definitely add these flags in the vscode CI rather than addressing them individually. The advantage of the current method is that we get to run the tests of these individual modules and make sure the flags does not regress any behavior whereas we can only guarantee compile time failures if we do it in vscode CI.

Also after going through our native module list, there are only two of them which we don't own currently and we have decided to ignore them from the BinSkim pipeline https://github.com/microsoft/vscode-internalbacklog/issues/2767.

deepak1556 commented 2 years ago

Should be rebased once https://github.com/microsoft/node-native-keymap/pull/40 gets merged, for testing.

TylerLeonhardt commented 2 years ago

@alexdima need anything else before merging?

alexdima commented 2 years ago

Continued in https://github.com/microsoft/vscode/pull/144669