-
-
Notifications
You must be signed in to change notification settings - Fork 516
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.
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
build: migrate from github actions to circleci #3188
Conversation
0816a75
to
0d5dc0f
Compare
e1063a8
to
723a533
Compare
npm install -g npm@8 | ||
echo 'C:\Program Files (x86)\WiX Toolset v3.11\bin' >> $GITHUB_PATH | ||
npm config set node-gyp "$GITHUB_WORKSPACE\node_modules\node-gyp\bin\node-gyp.js" | ||
- name: Linux specific setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was dead code, is this something we need?
@@ -143,11 +143,11 @@ | |||
"yaml-hook": "^1.0.0" | |||
}, | |||
"optionalDependencies": { | |||
"@malept/electron-installer-flatpak": "^0.11.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these aren't needed for this PR- let me know if it would be more preferred to separate these upgrades to a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with upgrading these in this PR, as long as tests are passing 👍 We can do a little manual test of binaries next time we do a Forge release, but the upgrade should be safe
@@ -27,7 +27,7 @@ | |||
"fs-extra": "^10.0.0" | |||
}, | |||
"optionalDependencies": { | |||
"@malept/electron-installer-flatpak": "^0.11.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
/** | ||
* If the `RunAsNode` fuse had not been flipped, | ||
* this would return the Node.js version (e.g. `v14.16.0`) | ||
* instead of the `console.log` from `main.js`. | ||
*/ | ||
const output = ( | ||
await spawn(electronExecutablePath, ['-v'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments to look into, but all in all, this is awesome work! Thanks so much for doing this conversion, there were some thorny debugging problems in here 🙇♀️
@@ -143,11 +143,11 @@ | |||
"yaml-hook": "^1.0.0" | |||
}, | |||
"optionalDependencies": { | |||
"@malept/electron-installer-flatpak": "^0.11.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with upgrading these in this PR, as long as tests are passing 👍 We can do a little manual test of binaries next time we do a Forge release, but the upgrade should be safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 small change. Thanks for this @georgexu99 !!!
This PR migrates Forge from GitHub Actions to CircleCI
Notes: none