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.

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

Add GitHub Actions workflow and delete old CI configurations #959

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tniessen
Copy link
Member

Not ready, but let's see what happens.

  • Fix build for Node.js 9 and below
  • Copy other version-dependent logic from .travis.yml and appveyor.yml as necessary
  • Build and test against Electron versions (see ELECTRON_VERSION in .travis.yml)
  • Delete .travis.yml and appveyor.yml

Fixes: #958

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 21, 2023 via email

@tniessen tniessen marked this pull request as draft October 21, 2023 17:29
@tniessen
Copy link
Member Author

I still need to fix a few things, see the list in the PR description. I'll ping you once that's done, hopefully soon.

@tniessen
Copy link
Member Author

This seems to work out-of-the-box for Node.js 10 and above, which includes all release lines of Node.js that have received any level of support since the end of 2019. On Node.js 9 and below, it seems that we need to at least downgrade node-gyp (see #932). However, older versions of node-gyp don't seem to fully support Python 3... and that might only be the tip of the iceberg.

I'd really like to see tests pass even against Node.js 0.10, but I am not sure how much benefit that actually brings in practice. I'd be glad to spend some more time on this if the maintainers of this repository feel that testing against Node.js 9 and below is important, but otherwise, perhaps Node.js 10 and above is already better than the broken Travis setup. (The only officially supported Node.js release lines are 18, 20, and 21 right now.)

strategy:
matrix:
os: [ubuntu-latest, windows-latest, macos-latest]
node-version: ['0.10', 0.12, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
node-version: ['0.10', 0.12, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21]
node-version: ['0.10', 0.12, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23]

Comment on lines +20 to +22
uses: actions/checkout@v3
- name: Set up Node.js ${{matrix.node-version}}
uses: actions/setup-node@v3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uses: actions/checkout@v3
- name: Set up Node.js ${{matrix.node-version}}
uses: actions/setup-node@v3
uses: actions/checkout@v4
- name: Set up Node.js ${{matrix.node-version}}
uses: actions/setup-node@v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use GitHub Actions and delete Travis / AppVeyor configuration
3 participants