Skip to content

Conversation

tcely
Copy link
Contributor

@tcely tcely commented Nov 25, 2024

The most important is catching when the s6 or ffmpeg binaries aren't actually working correctly.

tcely added 15 commits November 24, 2024 00:15
Specifically turn off flags to printf where none are intended.
There are fewer things to keep in mind this way.
Nothing later should have been affected, but this is the cleaner way.
This should be enough to fail the build when the binary doesn't work.
The shell used on the build system doesn't support `pipefail` so test for output instead.

It might be incorrect output, but in combination with running the `ffmpeg` binary earlier, this should be enough.
It turned out `exit` doesn't work as I thought it did.
@meeb
Copy link
Owner

meeb commented Nov 26, 2024

What build system are you using? I don't particularly mind merging these after looking at them, but a lot of them seem to be accounting for what look like largely impossible failures or dumping info that's likely not too useful.

Is there some use case for all of this?

@tcely
Copy link
Contributor Author

tcely commented Nov 26, 2024

Actually, the inspiration was from seeing the logs from the GitHub workflow.

I was surprised that the arm64 build wasn't failed before you merged the removal of =amd64 from TARGETARCH so I wanted to prevent a push of a container with incompatible ffmpeg from going unnoticed.

The change of the scope for when HOME was altered should not make a difference now, but if a later addition didn't account for that, it might cause confusion later. It's just better practice to limit which commands see that altered value, to as few as we need.

The mkdir output did answer a question I had about if some of the directories were existing already. The file output would have allowed me to see the type of binary that was extracted rather than inferring the problem in the older logs through the error or URL that was logged.

@meeb
Copy link
Owner

meeb commented Nov 26, 2024

Far enough, no real objections to your adjustments, I'll merge them into the next release.

Edit: and thanks for the efforts!

@meeb meeb merged commit 40e48c9 into meeb:main Nov 26, 2024
@tcely tcely deleted the patch-3 branch November 27, 2024 11:00
@tcely
Copy link
Contributor Author

tcely commented Nov 27, 2024

Thank you for your great software and prompt reviews!

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.

2 participants