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: detect failure to run make docomp nicely again #4421

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 20, 2021

We have a test for our CI which detects if a PR should have run make docomp.
But we did not reach it anymore since some changes to bin/BuildPackages.sh
which added code to use GAP to extract the name of any given packages, by
letting GAP print it to stdout and grabbing it from there. But in this
particular failure scenario, GAP also printed a warning, which caused
BuildPackages.sh to fail. This patch suppresses that warning.

To verify this works, I am adding a commit which is supposed to trigger this failure mode.

UPDATE: test worked as desired, see e.g. https://github.com/gap-system/gap/runs/2394724252?check_suite_focus=true

We have a test for our CI which detects if a PR should have run `make docomp`.
But we did not reach it anymore since some changes to `bin/BuildPackages.sh`
which added code to use GAP to extract the name of any given packages, by
letting GAP print it to stdout and grabbing it from there. But in this
particular failure scenario, GAP also printed a warning, which caused
`BuildPackages.sh` to fail. This patch suppresses that warning.
@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: ci Anything related to GitHub Actions, Codecov, AppVeyor, Coveralls, Travis, ... labels Apr 20, 2021
@fingolfin fingolfin marked this pull request as ready for review April 20, 2021 21:25
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Thanks.
(I had suggested this change in issue #4417.)
The test failure is again due to tst/testbugfix/2006-08-28-t00151.tst, see issue #4368.
We can merge the pull request, but the test is apparently still not safe.

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Thanks for the demonstration that this works.

I think we should merge despite the unrelated CI job failure.

@fingolfin fingolfin merged commit 7dcab96 into gap-system:master Apr 21, 2021
@fingolfin fingolfin deleted the mh/fix-docomp-ci branch April 21, 2021 10:29
@fingolfin
Copy link
Member Author

Test: reference #4368 from a regular PR comment

Copy link
Member Author

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Test: reference #4368 from a PR review comment

@fingolfin
Copy link
Member Author

Test: reference #4368 from a regular PR comment again

Copy link
Member Author

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Test: reference #4368 from a PR review comment again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: ci Anything related to GitHub Actions, Codecov, AppVeyor, Coveralls, Travis, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants