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

updating to macos-14 #1486

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dhchandw
Copy link
Collaborator

@dhchandw dhchandw commented Nov 20, 2024

also using 7zip pre-installed library instead of using node module

@dhchandw dhchandw marked this pull request as draft November 20, 2024 16:28
@dhchandw dhchandw marked this pull request as ready for review November 20, 2024 16:50
Copy link
Collaborator

@tecimovic tecimovic left a comment

Choose a reason for hiding this comment

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

I'm ok with it, but to check of this will work, since I thought you need npx there....

@@ -272,7 +272,7 @@ jobs:
- name: Verify zap-cli exists in Windows arm64 .zip package
if: startsWith(matrix.os, 'macos')
run: |
output=$(./node_modules/7zip-bin/mac/x64/7za l ./dist/zap-win-arm64.zip)
output=$(7za l ./dist/zap-win-arm64.zip)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that's what npx is for? Shouldn't you do npx 7za ? I am confused why are you changing this path.

It is true, that running ./node_modules like it was is wrong, but npx should do that for you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/actions/runner-images/blob/main/images/macos/macos-14-Readme.md

7zip comes pre-installed as a utility in the macos-14 environment. I could change it to use the node module though.

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