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 ghactions support #118

Merged
merged 6 commits into from
Dec 10, 2023
Merged

Add ghactions support #118

merged 6 commits into from
Dec 10, 2023

Conversation

4ver
Copy link
Member

@4ver 4ver commented Dec 6, 2023

  • Target platforms this affects (Linux, Mac, Mac app store, and or Windows):
  • What problem does this solve?
  • Could it break any existing functionality for users?

@Oxalin
Copy link
Collaborator

Oxalin commented Dec 7, 2023

"coffee-script" module must be updated to "coffeescript" (it was renamed) with recent versions. This should allow to run tests with Node.js 8 and 14.

However, for Node.js 4, I see this that probably needs to be fixed or telling us we shouldn't support Node.js 4: npm WARN engine [email protected]: wanted: {"node":">=6.0.0"} (current: {"node":"4.9.1","npm":"2.15.11"})

@Oxalin Oxalin self-requested a review December 7, 2023 07:06
Oxalin
Oxalin previously approved these changes Dec 7, 2023
@Oxalin
Copy link
Collaborator

Oxalin commented Dec 7, 2023

Also, I would suggest to test against LTS releases that are still supported or the ones that were last pushed out of maintenance. This means 16, 18 and 20.

@Oxalin Oxalin self-requested a review December 7, 2023 07:45
@Oxalin
Copy link
Collaborator

Oxalin commented Dec 7, 2023

gulp-coffeelint has not been updated for the last 8 years and still depends on "coffee-script" (with a "-")...

@Oxalin Oxalin dismissed their stale review December 7, 2023 07:54

I wanted to approve a commit, not the series.

@Oxalin
Copy link
Collaborator

Oxalin commented Dec 8, 2023

Also, for Node >= 10, we need to move to gulp 4 (that is why I had the "natives" modules in my commit series because it was a workaround that some people had identified) and update whatever needs to be updated: https://davidsekar.com/nodejs/upgrading-your-gulp-for-running-with-node-v10

@Oxalin
Copy link
Collaborator

Oxalin commented Dec 9, 2023

We seem to have a problem under macOS where the CI testing fails to .enable() autolaunch Calculator, and thus fails to disable it. Was it passing before?

Copy link
Collaborator

@Oxalin Oxalin left a comment

Choose a reason for hiding this comment

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

Looks good and tested. Works on Linux and Windows.

I'm worried about the macOS tests failing, but it seems to have nothing to do with the changes in this PR.

jobs:
build:
strategy:
matrix:
os: [macos-latest, windows-latest, ubuntu-latest]
node-version: ["0.12", "4.9", "8.17", "14.0"]
node-version: ["4", "8", "14"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop 4 and 8 at least and add 18 and 20 instead. I don't know about 14, it doesn't receive security support anymore since last April. And for the missing 16, it doesn't receive security support either since last September, but it's the last LTS version out the door, so I don't know. Maybe add it for now.

Fix conflicts between branches.

Signed-off-by: Alexandre Demers <[email protected]>
@Oxalin Oxalin merged commit df6b3b6 into master Dec 10, 2023
0 of 15 checks passed
@4ver
Copy link
Member Author

4ver commented Dec 11, 2023

We seem to have a problem under macOS where the CI testing fails to .enable() autolaunch Calculator, and thus fails to disable it. Was it passing before?

This was working on older versions of macos afaik. In more recent versions only the Launch Agent way works.

@4ver
Copy link
Member Author

4ver commented Dec 11, 2023

Not sure it was a good idea to merge this as the tests aren't passing 😅

@Oxalin
Copy link
Collaborator

Oxalin commented Dec 11, 2023

Not sure it was a good idea to merge this as the tests aren't passing 😅

I though it was already failing prior to your PR. My bad. I'll review your new PR.

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

Successfully merging this pull request may close these issues.

2 participants