search

flatpak / flatpak-builder

Tool to build flatpaks from source
GNU Lesser General Public License v2.1
141 stars 93 forks source link

build-commands: add: The build process fails when an individual entry… #601

Closed nandedamana closed 6 months ago

nandedamana commented 6 months ago

… in the array fails.

Just making it clear so that one won't be hesitant to break down a long '&&'-ed one-liner.

smcv commented 6 months ago

Just making it clear so that one won't be hesitant to break down a long '&&'-ed one-liner.

This is good reasoning that ought to be captured in the commit message for the benefit of future maintainers. (For example see https://cbea.ms/git-commit/)

smcv commented 6 months ago

Please squash your two commits into one, ideally with an explanatory commit message like the one I suggested in https://github.com/flatpak/flatpak-builder/pull/601#pullrequestreview-2066126671

nandedamana commented 6 months ago

Just making it clear so that one won't be hesitant to break down a long '&&'-ed one-liner.

This is good reasoning that ought to be captured in the commit message for the benefit of future maintainers. (For example see https://cbea.ms/git-commit/)

Yes, thanks. I was in a hurry and was just copy-pasting something relevant from the change itself.

BTW, this commit originated from a similar hesitation that I had and a YAML pitfall that made me question the behaviour of build-commands. It is documented here: https://nandakumar.org/blog/2024/05/yaml-boolean-trap.html

smcv commented 6 months ago

Yeah, it's clearly a bug that flatpak-builder doesn't run false as a command in that situation, but also doesn't error out with some semi-useful message like "expected string, but found boolean". Please report that as an issue if nobody has done so already - and I'm sure a PR to solve it would be appreciated, if you have some time.

false is not even the worst YAML trap: if you're discussing ISO country codes, it's easy to get tripped up by Norway being no, which (if unquoted) also gets interpreted as boolean FALSE ("the Norway problem").

nandedamana commented 6 months ago

Please squash your two commits into one, ideally with an explanatory commit message like the one I suggested in #601 (review)

Your commit message looked fine, so I just reused it.

nandedamana commented 6 months ago

@smcv Anything else to be done from my part?