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

remove setup-perl action from windows jobs #22555

Closed
wants to merge 1 commit into from

Conversation

quarckster
Copy link
Contributor

@quarckster quarckster commented Oct 30, 2023

Windows runners have Perl preinstalled.
https://github.com/actions/runner-images/blob/main/images/win/Windows2022-Readme.md

Checklist
  • documentation is added or updated
  • tests are added or updated

@quarckster quarckster changed the title remove setup-perl actiond from windows jobs remove setup-perl action from windows jobs Oct 30, 2023
@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Oct 30, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 31, 2023
@h-vetinari
Copy link
Contributor

Sidenote: the default perl in the runner images drags in much of GCC onto the PATH, which is obviously a source of great fun on windows (and something that both upstream strawberry perl as well as GHA have refused to fix). It might ultimately be cleaner to depend on a less troublesome perl, even if it's part of the image.

@quarckster
Copy link
Contributor Author

Sidenote: the default perl in the runner images drags in much of GCC onto the PATH, which is obviously a source of great fun on windows (and something that both upstream strawberry perl as well as GHA have refused to fix). It might ultimately be cleaner to depend on a less troublesome perl, even if it's part of the image.

I'm not sure how the action shogo82148/actions-setup-perl solves this problem. Does it delete preinstalled Perl or change PATH?

@quarckster
Copy link
Contributor Author

quarckster commented Oct 31, 2023

Sidenote: the default perl in the runner images drags in much of GCC onto the PATH, which is obviously a source of great fun on windows (and something that both upstream strawberry perl as well as GHA have refused to fix). It might ultimately be cleaner to depend on a less troublesome perl, even if it's part of the image.

I'm not sure how the action shogo82148/actions-setup-perl solves this problem. Does it delete preinstalled Perl or change PATH?

Answering to myself. Actually, yes https://github.com/shogo82148/actions-setup-perl/blob/main/src/strawberry.ts#L78 but if you install strawberry Perl from the action.

@h-vetinari
Copy link
Contributor

I'm not sure how the action shogo82148/actions-setup-perl solves this problem.

If things work as in this PR, that's alright I guess. It was more of a heads-up about the idiosyncrasies of the built-in perl.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 1, 2023
@t8m
Copy link
Member

t8m commented Nov 1, 2023

Merged to the master branch. Thank you.

@t8m t8m closed this Nov 1, 2023
openssl-machine pushed a commit that referenced this pull request Nov 1, 2023
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants