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

Pass -f to rm in cleanup handlers #2069

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

wantehchang
Copy link
Collaborator

When a test fails, some of the files the cleanup handler needs to remove may not have been created, so pass -f to rm to ignore nonexistent files.

When a test fails, some of the files the cleanup handler needs to remove
may not have been created, so pass -f to rm to ignore nonexistent files.
@wantehchang wantehchang requested review from y-guyon and vrabaud March 19, 2024 23:25
@wantehchang
Copy link
Collaborator Author

In https://github.com/AOMediaCodec/libavif/actions/runs/8351405561/job/22859682384?pr=2068, search for "No such" in the logs, you will see error messages from the rm command like the following:

+ /home/runner/work/libavif/libavif/build/avifenc -s 8 /home/runner/work/libavif/libavif/tests/data/paris_icc_exif_xmp.png --grid 7x5 -o avif_test_cmd_grid_7x5_encoded.avif
/home/runner/work/libavif/libavif/tests/test_cmd_grid.sh: line 74:  7339 Segmentation fault      (core dumped) "${AVIFENC}" -s 8 "${INPUT_PNG}" --grid 7x5 -o "${ENCODED_FILE_7x5}"
+ cleanup
+ pushd /tmp/tmp.77nsVRNFde
/tmp/tmp.77nsVRNFde /tmp/tmp.77nsVRNFde ~/work/libavif/libavif/build/tests
+ rm -- avif_test_cmd_grid_encoded.avif avif_test_cmd_grid_decoded.png avif_test_cmd_grid_2x2_encoded.avif avif_test_cmd_grid_2x2_decoded.png avif_test_cmd_grid_7x5_encoded.avif avif_test_cmd_grid_7x5_decoded.png
rm: cannot remove 'avif_test_cmd_grid_encoded.avif': No such file or directory
rm: cannot remove 'avif_test_cmd_grid_decoded.png': No such file or directory
rm: cannot remove 'avif_test_cmd_grid_7x5_encoded.avif': No such file or directory
rm: cannot remove 'avif_test_cmd_grid_7x5_decoded.png': No such file or directory

Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

I was using that to see if some args given to rm were out-of-date, but I agree it clutters the logs (on test failure only though).

Could we have -f only on trap and not on exit success? Probably not worth the effort though. Something like:

# Assume no space in paths
FILES_TO_RM="${ENCODED_FILE}" "${DECODED_FILE}" "${ERROR_MSG}"
cleanup() {
  pushd ${TMP_DIR}
    rm -f -- ${FILES_TO_RM}
  popd
}

...
rm -- ${FILES_TO_RM} # Test should fail if a file is missing
exit 0

Feel free to merge this PR as is.

@wantehchang
Copy link
Collaborator Author

Yannis: I also noticed that rm -f prevents us from knowing the list of files to delete has become out of date.

I can abandon this pull request. Would you like to implement the approach you outlined?

@y-guyon
Copy link
Collaborator

y-guyon commented Mar 20, 2024

I can abandon this pull request. Would you like to implement the approach you outlined?

Not now, you can merge as is, thank you.

@wantehchang wantehchang merged commit 4c644d5 into AOMediaCodec:main Mar 20, 2024
21 checks passed
@wantehchang wantehchang deleted the rm-f-in-cleanup branch March 20, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants