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

docs: Make appimage examples consistent with --appimage option short description #5402

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

slowpeek
Copy link
Contributor

@slowpeek slowpeek commented Oct 5, 2022

--appimage option is mentioned twice in the man page: first with a short description, next with usage examples. The short description is:

firejail [OPTIONS] --appimage [appimage-file and arguments]

While the examples are of form firejail --appimage <some extra options> someapp.appimage.

Even though the option parser recognizes such order, it is better to keep it the same way in both places in the man page.

@kmk3 kmk3 changed the title Make appimage examples consistent with --appimage option short description docs: Make appimage examples consistent with --appimage option short description Oct 5, 2022
@kmk3 kmk3 added the documentation Issues and pull requests related to the documentation label Oct 5, 2022
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

Good catch!

LTGM.

@netblue30 netblue30 merged commit 32e8175 into netblue30:master Oct 11, 2022
@netblue30
Copy link
Owner

Merged, thanks for the fix.

@rusty-snake
Copy link
Collaborator

rusty-snake commented Oct 11, 2022

FWIW: The problem with this post notation of --appimage is that the HAS_APPIMAGE condition will be false.

kmk3 added a commit to kmk3/firejail that referenced this pull request Nov 4, 2022
And fix the argument order in the examples to reflect that.

Background: The order in which these options appeared in the
documentation was inconsistent.  src/man/firejail.txt used --appimage
before --profile and src/man/firejail-profile.txt used --profile before
--appimage.  Then commit 44fefca ("Make appimage examples consistent
with --appimage option short description", 2022-10-05) / PR netblue30#5402 was
made, which standardized on --profile before --appimage in both places.

But as mentioned by @rusty-snake[1], --appimage has be specified before
--profile in order for any `?HAS_APPIMAGE` conditionals inside of the
profile to evaluate to true.

So change the documentation to use and recommend the latter form.

Also, add --quiet to one example to make it clear that --appimage does
not have to be the first option (nor the last option before --profile).

[1] netblue30#5402 (comment)
kmk3 added a commit that referenced this pull request Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues and pull requests related to the documentation
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

4 participants