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

Use custom node-gyp for SDL pipeline #179723

Merged
merged 11 commits into from
Apr 21, 2023
Merged

Use custom node-gyp for SDL pipeline #179723

merged 11 commits into from
Apr 21, 2023

Conversation

rzhao271
Copy link
Contributor

No description provided.

@rzhao271 rzhao271 added this to the April 2023 milestone Apr 11, 2023
@rzhao271 rzhao271 self-assigned this Apr 11, 2023
@rzhao271
Copy link
Contributor Author

The VS Code Scan failed due to Binskim, which is currently being resolved elsewhere.
What is relevant for the scan results is that the dependencies are being installed and compiled properly.

@rzhao271 rzhao271 enabled auto-merge (squash) April 11, 2023 22:59
@rzhao271
Copy link
Contributor Author

rzhao271 commented Apr 12, 2023 •
edited
Loading

Building the custom node-gyp is still good, so the CI passes those steps, but it fails in general because it's missing symbols, still.
CI run: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=210534&view=results

We can merge this PR in and wait for the symbols, or we can wait for the symbols first.

@deepak1556
Copy link
Collaborator

Lets wait for the pdb files and get a green CI before merging.

@deepak1556
Copy link
Collaborator

Pdb are now available starting with https://github.com/microsoft/vscode-electron-prebuilt/releases/tag/22.4.5, please rebase on top of main once #179857 gets merged.

@rzhao271
Copy link
Contributor Author

Symbols are still not being downloaded. Looks like the scan gulp file might be a bit broken. Investigating.

@rzhao271
Copy link
Contributor Author

This took me way too long to figure out.
Upstream PR: joaomoreno/gulp-atom-electron#80

@rzhao271 rzhao271 disabled auto-merge April 14, 2023 01:49
@rzhao271 rzhao271 enabled auto-merge (squash) April 14, 2023 04:13
@deepak1556
Copy link
Collaborator

👏 spectre flag is only raised for Electron related binaries and keytar as expected.

I am good with the changes of this PR but lets wait for @joaomoreno who will be back on Monday and we can get a new version of gulp-atom-electron and we can switch to it instead.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Apr 20, 2023 •
edited
Loading

We're now using @vscode/gulp-electron!
The SDL scan continues to pass with keytar.node marked as the only native node module without Spectre mitigation added.

SDL scan: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=211604&view=results
Raw Binskim log for that scan: https://dev.azure.com/monacotools/a6d41577-0fa3-498e-af22-257312ff0545/_apis/build/builds/211604/logs/55

@rzhao271 rzhao271 merged commit ee76f10 into main Apr 21, 2023
@rzhao271 rzhao271 deleted the rzhao271/sdl-custom branch April 21, 2023 00:15
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees

@rzhao271 rzhao271

Labels
None yet
Projects
None yet
Milestone
April 2023
Development

Successfully merging this pull request may close these issues.
2 participants
@rzhao271 @deepak1556