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

CI: Run logformatter for podman_machine_windows_task #21991

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Mar 8, 2024

That's to address the windows part of #21760

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 8, 2024
@l0rd l0rd force-pushed the logformatter-for-win branch 11 times, most recently from c26e30b to 2a6b7a1 Compare March 11, 2024 13:56
@l0rd l0rd marked this pull request as ready for review March 11, 2024 13:56
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2024
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks, logs look much better with that

Comment on lines 37 to 38
Write-Host "`nInstalling Perl as it is required to use logformatter"
Run-Command 'choco install --no-progress --confirm --acceptlicense --nocolor StrawberryPerl'
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we are checking if Perl exists before invoking logformatter and install it if necessary:

function Install-Perl-If-Required {
if (-not (Get-Command perl -ErrorAction SilentlyContinue)) {
Write-Host "`nInstalling Perl as it is required to use logformatter"
choco install --no-progress --confirm --acceptlicense --nocolor StrawberryPerl
Write-Host "`nAdd perl to the PATH"
Set-Item "Env:PATH" "$Env:PATH;C:\Strawberry\perl\bin"
}
}

I will open another PR to remove this code as soon as Perl is available in the new windows image.

Copy link
Member

Choose a reason for hiding this comment

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

contrib/cirrus/win-podman-machine-test.ps1 Outdated Show resolved Hide resolved
@l0rd l0rd marked this pull request as draft March 12, 2024 13:40
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2024
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Mar 18, 2024
New pasta (03-18). And, probably, lots more changes, some of
which will break CI.

Also: install StrawberryPerl on Windows, see:

  containers/podman#21991

(Note: debian crun 1.14.3 still does not exist. Grrr.)

First CI-detected problem:

    debian: The following packages have unmet dependencies:
    debian:  libfuse2t64 : Breaks: libfuse2 (< 2.9.9-8.1)

Solution attempted: remove libfuse2 from INSTALL_PACKAGES

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Member

Thank you! I'm working on logformatter, adding code to recognize C: and backslashes and that sort of thing.

One request: could you find some way to inject /define.gitCommit=XXXXX into the log output? Without that there's no way to linkify back to source code. Here's how I did it for Macintosh:

podman/Makefile

Lines 632 to 636 in 8a643c2

.PHONY: localmachine
localmachine:
# gitCommit needed by logformatter, to link to sources
@echo /define.gitCommit=$(GIT_COMMIT)
$(MAKE) ginkgo-run GINKGO_PARALLEL=n TAGS="$(REMOTETAGS)" GINKGO_FLAKE_ATTEMPTS=0 FOCUS_FILE=$(FOCUS_FILE) GINKGOWHAT=pkg/machine/e2e/. HACK=

...but of course that relies on an invocation of the form make localmachine |& logformatter

@edsantiago
Copy link
Member

@l0rd feel free to grab #22081

edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Mar 19, 2024
New pasta (03-19). And whatever else comes in.

Also: install StrawberryPerl on Windows, see:

  containers/podman#21991

First CI-detected problem:

    debian: The following packages have unmet dependencies:
    debian:  libfuse2t64 : Breaks: libfuse2 (< 2.9.9-8.1)

Solution attempted: remove libfuse2 from INSTALL_PACKAGES

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Mar 20, 2024
New pasta (03-19 and -20). And whatever else comes in.

Also: install StrawberryPerl on Windows, see:

  containers/podman#21991

First CI-detected problem:

    debian: The following packages have unmet dependencies:
    debian:  libfuse2t64 : Breaks: libfuse2 (< 2.9.9-8.1)

Solution attempted: remove libfuse2 from INSTALL_PACKAGES

And, bump expired Debian timebombs

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Mar 20, 2024
New pasta (03-20). And whatever else comes in.

Also: install StrawberryPerl on Windows, see:

  containers/podman#21991

First CI-detected problem:

    debian: The following packages have unmet dependencies:
    debian:  libfuse2t64 : Breaks: libfuse2 (< 2.9.9-8.1)

Solution attempted: remove libfuse2 from INSTALL_PACKAGES

And, bump expired Debian timebombs

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Mar 20, 2024
New pasta (03-20). And whatever else comes in.

Also: install StrawberryPerl on Windows, see:

  containers/podman#21991

First CI-detected problem:

    debian: The following packages have unmet dependencies:
    debian:  libfuse2t64 : Breaks: libfuse2 (< 2.9.9-8.1)

Solution attempted: remove libfuse2 from INSTALL_PACKAGES

And, bump expired Debian timebombs

Signed-off-by: Ed Santiago <santiago@redhat.com>
@l0rd
Copy link
Member Author

l0rd commented Mar 26, 2024

I have update the PR so that the html report is generated when tests fail too (this was tricky because there is no way to set pipefail with powershell as we do on bash scripts).

Also now the reports first line has the git commit: /define.gitCommit=<SHA>

Now the windows cirrus tasks install Perl if it's not installed yet. We can merge the PR as is or wait for the new VMs with Perl and remove the code that installs Perl from this PR.

Tests failing examples:

Tests passing examples:

@l0rd l0rd marked this pull request as ready for review March 26, 2024 16:11
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2024
.cirrus.yml Outdated
always:
# Required for `contrib/cirrus/logformatter` to work properly
html_artifacts:
path: ./repo/*.html
Copy link
Member

Choose a reason for hiding this comment

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

The logs look great! Thank you! But this spurious /repo breaks all existing conventions, and makes the logs impossible to find. Is there any way to remove the /repo?

Copy link
Member

Choose a reason for hiding this comment

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

Sigh. Doesn't look like there's a way to do that with Cirrus. Can the file be written (or moved) to ..?

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 ./repo/ folder is a peculiarity of the windows build (and it took some time to figure that out). I can have a look at the root cause of it. I would like to set ./repo as the working directory to make it consistent with the other platforms. Otherwise I can move the html report up one folder when the log formatter terminates.

Copy link
Member

Choose a reason for hiding this comment

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

Without a 1-click link the formatted logs are not really very useful. However, it's still incremental progress and maybe other Windows peeps will want it.

@edsantiago
Copy link
Member

Also, you might not know about this: https://github.com/edsantiago/greasemonkey/tree/master/github-ci-highlight

In addition to colorizing github jobs in a way that makes patterns stand out, it also auto-links to colorized logs, adding a single-click trivial way to jump straight to logs. With the /repo thing, this linkification no longer works. Nor does my flake logger.

Copy link
Contributor

openshift-ci bot commented Mar 26, 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2024
@l0rd l0rd force-pushed the logformatter-for-win branch 3 times, most recently from 810ac29 to 42b45c0 Compare March 26, 2024 23:55
@l0rd l0rd marked this pull request as draft March 27, 2024 10:20
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2024
containers#21760

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd l0rd marked this pull request as ready for review March 27, 2024 13:08
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2024
@baude
Copy link
Member

baude commented Mar 27, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024
@l0rd
Copy link
Member Author

l0rd commented Mar 27, 2024

Also, you might not know about this: edsantiago/greasemonkey@master/github-ci-highlight

In addition to colorizing github jobs in a way that makes patterns stand out, it also auto-links to colorized logs, adding a single-click trivial way to jump straight to logs. With the /repo thing, this linkification no longer works. Nor does my flake logger.

Thanks for sharing. I was not aware of it.

I made an update to the PR to move the report one folder up and now the link works.

@edsantiago
Copy link
Member

Confirmed, the link now works. Thank you!

@openshift-merge-bot openshift-merge-bot bot merged commit a77f705 into containers:main Mar 27, 2024
93 of 94 checks passed
edsantiago added a commit to edsantiago/libpod that referenced this pull request Apr 3, 2024
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 added a commit to edsantiago/libpod that referenced this pull request Apr 3, 2024
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>
@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 3, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 3, 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. machine release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants