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

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

Merged

Conversation

nandedamana
Copy link
Contributor

… in the array fails.

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

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

Thanks, it would be good to document this behaviour.

The first line of the commit message is rather long (and Github is giving you a hint that this might be a problem by ellipsizing it). Maybe something like this?

flatpak-manifest(5): If one command fails, the whole build fails

Documenting this makes it more obvious that it's valid to write

    - foo
    - bar
    - baz

and unnecessary to combine those commands into
`foo && bar && baz` to get sensible error behaviour.

doc/flatpak-manifest.xml Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented May 20, 2024

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
Copy link
Collaborator

smcv commented May 20, 2024

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

@nandedamana
Copy link
Contributor Author

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
Copy link
Collaborator

smcv commented May 20, 2024

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").

Documenting this makes it more obvious that it's valid to write

    - foo
    - bar
    - baz

and unnecessary to combine those commands into
`foo && bar && baz` to get sensible error behaviour.

Co-authored-by: Simon McVittie <smcv@collabora.com>
@nandedamana nandedamana force-pushed the nan-build-commands-doc-explain-fail branch from 68f99ac to 45b225b Compare May 20, 2024 13:10
@nandedamana
Copy link
Contributor Author

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
Copy link
Contributor Author

@smcv Anything else to be done from my part?

@TingPing TingPing merged commit 61e3a2e into flatpak:main May 23, 2024
5 checks passed
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.

None yet

3 participants