Page MenuHomeFreeBSD

libarchive: Make test_read_append_filter_wrong_program pass again
ClosedPublic

Authored by arichardson on Mar 3 2021, 11:46 AM.

Details

Summary

libarchive: Apply upstream commit a1b7bf8013fb7a11a486794247daae592db6f5ae

This fixes the failing test_read_append_filter_wrong_program test in CI.

Commit message from https://github.com/libarchive/libarchive/commit/a1b7bf8013fb7a11a486794247daae592db6f5ae

Silence stderr in test_read_append_filter_program

When the FreeBSD testsuite runs the libarchive tests it checks that stderr
is empty. Since #1382 this is no longer the case. This change restores
the behaviour of silencing bunzip2 stderr but doesn't bring back the
output text check.
 
Partially reverts 2e7aa5d9
Test Plan

Test now passes

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

What about we develop a fix for this at upstream?

In D29036#650248, @mm wrote:

What about we develop a fix for this at upstream?

Sure, I guess we could also silence stderr upstream? I assumed that printing something on stderr was a deliberate upstream decision.

@mm Alternatively we could just ignore whatever is printed on stderr (-e ignore) here? All we really care about is that the test exits with exit code zero? That would be more future-proof since there could be tests in the future that print debug information on stderr (especially since we are running tests with the -v flag).

If you don't know I am a core libarchive developer and release manager so getting the fix upstream is up to me.
I don't like modifying the framework this way, we should fix the individual test instead.

In D29036#653313, @mm wrote:

If you don't know I am a core libarchive developer and release manager so getting the fix upstream is up to me.
I don't like modifying the framework this way, we should fix the individual test instead.

Yes I just wasn't sure whether empty stderr is guaranteed by libarchive tests or not. It's not for the current version. Since you merged https://github.com/libarchive/libarchive/pull/1382 I thought maybe empty stderr is not required by the tests.

I've filed https://github.com/libarchive/libarchive/pull/1505. If you think that is the correct way forward I will drop this revision and wait for the next libarchive update.

Thank you. We are testing Linux, FreeBSD, macOS (via GitHub Actions) and OmniOS (via Jenkins) on GitHub so that will tell how it behaves on other platforms.

In D29036#653316, @mm wrote:

Thank you. We are testing Linux, FreeBSD, macOS (via GitHub Actions) and OmniOS (via Jenkins) on GitHub so that will tell how it behaves on other platforms.

@mm In order to get stable-13 and HEAD CI green again we should either merge this or an upstream fix. CI has been broken for many months and I would really like to be able to catch regressions again.
Any objections against merging this until an upstream fix lands and is applied to contrib? The test will start failing again as soon as the upstream fix is committed so we will know when this workaround can be removed again.

@mm thanks for merging the PR. Should I apply that change to contrib/ or are you planning to do a vendor import soon?

Cherry-pick upstreamed commit instead

arichardson added a reviewer: tests.
This revision was not accepted when it landed; it landed in state Needs Review.Apr 7 2021, 10:37 AM
This revision was automatically updated to reflect the committed changes.