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

fix(workflows): exclude some node versions for windows/mac, update tools as needed for test #140

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

SlyryD
Copy link
Contributor

@SlyryD SlyryD commented Oct 22, 2024

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

  • The test workflow for mac is broken because osx-arm64 versions of Node.js don't exist before v16.1.0. Disable macos tests for node versions 10, 12, and 14.

  • New versions of python 3 leave out setuptools needed by node-gyp, so make sure those are installed for the test workflow.

  • Since the windows image has VS2022, we need node-gyp >= 8.x. The node-gyp 8.x dependencies do not support Node.js 10.x, so disable that node version for Windows. For Node.js 12.x and 14.x, update node-gyp to 8.x to get tests running.

NOTE: The macos-latest image now uses arm64, while the macos-latest-large image that uses x64 is available. When no architecture is specified for setup-node, it will use the system architecture, so by using an x64 mac image, it should successfully find Node 10.x, 12.x, 14.x, etc. However, the macos-latest-large image appears to require payment.

Copy link

linux-foundation-easycla bot commented Oct 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: SlyryD / name: Ryan Dorson (779f48d)

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 23, 2024

Ping @ersachin3112 to kick off the workflows so we can see whether this change fixes them. Thanks!

@alexander-akait
Copy link
Member

Weird, github requires payment, although actions are free for us

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 23, 2024

Weird, github requires payment, although actions are free for us

Is that related specicially to the macos-latest-large image?

@alexander-akait
Copy link
Member

I think yes...

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 23, 2024

I think yes...

Updated just to exclude node versions 10-14 in the macos test workflow.

@SlyryD SlyryD changed the title fix(workflows): support mac test workflow with old versions of node by using x64 image fix(workflows): exclude node versions that don't have arm distributions from mac test workflow Oct 23, 2024
@alexander-akait
Copy link
Member

alexander-akait commented Oct 23, 2024

Let's remove sudo npm i -g npm at all from CI

@evenstensberg
Copy link
Member

Yea sudo is a bad idea

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 24, 2024

Travelling now, will try again next week.

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 28, 2024

Let's remove sudo npm i -g npm at all from CI

Updated

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a0ed6c7) to head (779f48d).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #140   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           12        12           
  Branches         3         3           
=========================================
  Hits            12        12           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SlyryD
Copy link
Contributor Author

SlyryD commented Oct 30, 2024

New iteration where I force Python 3.12 for node-gyp, which has the distutils module. Alternative is to do one of the following to install distutils for newer versions of Python:

python3 -m pip install setuptools
brew install python-setuptools

But I figured that would take more iterations to get right.

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 11, 2024

New iteration where I force Python 3.12 for node-gyp, which has the distutils module. Alternative is to do one of the following to install distutils for newer versions of Python:

python3 -m pip install setuptools
brew install python-setuptools

But I figured that would take more iterations to get right.

@alexander-akait, new iteration with installing python setuptools. Didn't realize that I didn't git add my files 2 weeks ago.

@SlyryD SlyryD force-pushed the runTestOnX64Image branch 2 times, most recently from bf31da1 to 5952787 Compare November 11, 2024 23:35
@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

Ah I've seen this Windows build failure before with finding the VS2022 version... but I found a workaround that I can't explain why it works. So I'd have to dive deeper for this one.

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

Ah I've seen this Windows build failure before with finding the VS2022 version... but I found a workaround that I can't explain why it works. So I'd have to dive deeper for this one.

I'm thinking this might be why the original pipelines upgraded the version of npm...

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

Ah I've seen this Windows build failure before with finding the VS2022 version... but I found a workaround that I can't explain why it works. So I'd have to dive deeper for this one.

I'm thinking this might be why the original pipelines upgraded the version of npm...

Yeah so latest node 14 has npm 6.14.18 which has node-gyp 5.1.1, and according to this VS2022 support was added in node-gyp 8.4.0: nodejs/node-gyp#2958. Let me try something like this for Windows: https://github.com/nodejs/node-gyp/blob/main/docs/Updating-npm-bundled-node-gyp.md

@SlyryD SlyryD changed the title fix(workflows): exclude node versions that don't have arm distributions from mac test workflow fix(workflows): exclude some node versions for windows/mac, update tools as needed for test Nov 12, 2024
@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

@alexander-akait, ready for another try :D Since the windows image has VS2022, we need node-gyp >= 8.x. The node-gyp 8.x dependencies do not support Node.js 10.x, so rule that version out for Windows. For Node.js 12.x and 14.x, update node-gyp to 8.x to get tests running.

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

Dang... I got the same error locally in Windows but only when I had some NPM environment variables set. I guess they are set on the Windows image as well.

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

@alexander-akait, I have another iteration that tries to mimic these instructions better. Hope it works!

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 12, 2024

Yay! I'm not sure why macos-latest - Node v12.x and macos-latest - Node v14.x are pending though.

@SlyryD
Copy link
Contributor Author

SlyryD commented Nov 13, 2024

@alexander-akait, I think you would need to update the branch protection rules to remove the requirement for mac node 12 and 14 checks. I see other repos owned by webpack-contrib requiring node 18, 20, and 22 for example. Thanks!

@alexander-akait alexander-akait merged commit 24aee0d into webpack-contrib:master Nov 13, 2024
22 checks passed
@alexander-akait
Copy link
Member

Thanks a lot

@SlyryD SlyryD deleted the runTestOnX64Image branch November 13, 2024 19:33
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.

3 participants