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

Windows: clean up temporary perl install #22248

Merged

Conversation

edsantiago
Copy link
Member

Followup to #21991. Strawberry Perl is now installed by default
in CI VMs[1], so we no longer need the temporary install-perl code.

[1] containers/automation_images#337

Signed-off-by: Ed Santiago santiago@redhat.com

None

@edsantiago edsantiago requested a review from l0rd April 3, 2024 13:57
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 3, 2024
Write-Host "Invoking Logformatter"
Set-Item "Env:PATH" "$Env:PATH;C:\Strawberry\perl\bin"
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker if that's required for perl to work. But adding it to the PATH should be part of the installation.

Copy link
Member Author

Choose a reason for hiding this comment

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

The case-insensitive expression env.path does not appear anywhere in the automation_images repo (with one false positive). This suggests that all the other things that get installed by automation_images:

retryInstall git archiver psexec golang mingw

...are automatically installed into a proper place where they are visible in the caller's PATH. Do you know why Strawberry Perl is different? Or, taking a step back, is it possible that this Set-Item is not even necessary? Should I resubmit the PR and see if it works?

Copy link
Member

@l0rd l0rd Apr 3, 2024

Choose a reason for hiding this comment

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

As for the rest of the packages installed via chocolatey it should not be needed (the installer does that for us). But that's depends on the installer implementation.

I had to add it in my PR to be able to run the installer and then run perl from the same shell session. Now perl is installed at build time so the CI spawns a shell session where perl should already be in PATH.

I would try to resubmit the PR to see if it works yes.

Followup to containers#21991. Strawberry Perl is now installed by default
in CI VMs[1], so we no longer need the temporary install-perl code.

 [1] containers/automation_images#337

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the windows-perl-is-in-vms branch from f751c76 to ff133a5 Compare April 3, 2024 14:54
@edsantiago
Copy link
Member Author

Confirmation: wsl-windows log.

Copy link
Member

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, l0rd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Apr 4, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 3c71471 into containers:main Apr 4, 2024
94 checks passed
@edsantiago edsantiago deleted the windows-perl-is-in-vms branch April 4, 2024 09:24
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 4, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants