-
Notifications
You must be signed in to change notification settings - Fork 557
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
Create bcompare.profile #4098
Create bcompare.profile #4098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some nitpicks to go.
- add it to src/firecfg/firecfg.config
- if it has it's own configuration files/dirs, add them to disable-programs.inc
# Uncommenting this breaks launch | ||
# include disable-shell.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Uncommenting this breaks launch | |
# include disable-shell.inc |
no need to keep such lines (in general it's often enough to include allow-bin-sh.inc
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems a bit arbitrary... disable all shells, but then enable back bash
, dash
, and sh
, but not the much more popular zsh
. Why bother disabling shells then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zsh is popular as interactive shell yes, but not to write script in it as you can not assume it's installed on system. Usually you write you scripts for /bin/sh
which should be present on any POSIX compatible system or /bin/bash
which should be present on the most GNU/Linux systems. allow-bin-sh.inc
does exactly what it names says, it allows /bin/sh
. /bin/sh
is a symlink to /bin/bash
on the most system resp. /bin/dash
on Debian/Ubuntu. Therefore they need a noblacklist
too.
# Don't disable ${DOCUMENTS}, ${MUSIC}, ${PICTURES}, ${VIDEOS} | ||
# include disable-xdg.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Don't disable ${DOCUMENTS}, ${MUSIC}, ${PICTURES}, ${VIDEOS} | |
# include disable-xdg.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept such lines because to me it was surprising to realize that xdg
was related to ${DOCUMENTS}
, and I thought others who base the profile on this one might benefit from the comment. For the same reason, I added potentially superfluous comments like
# Might break Pulse Audio
#machine-id
or
# Allow associated applications to play sound files
#nosound
etc/profile-a-l/bcompare.profile
Outdated
# In case the user decides to include disable-programs.inc, still allow KDE's Gwenview to view images via right click -> Open With -> Associated Application | ||
noblacklist ${HOME}/.config/gwenviewrc | ||
|
||
include disable-common.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that we use the same comment format as in meld.profile and kdiff3.profile: Uncomment the next line (or put it into your meld.local) if you don't need to compare files in FOO
include disable-common.inc | |
# Uncomment the next line (or put it into your meld.local) if you don't need to compare files in disable-common.inc | |
#include disable-common.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed with meld
replaced by bcompare
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to use this message for the others (disable-programs.inc, wusc, ..) too.
Co-authored-by: rusty-snake <[email protected]>
Co-authored-by: rusty-snake <[email protected]>
Co-authored-by: rusty-snake <[email protected]>
Co-authored-by: rusty-snake <[email protected]>
Added those instructions to the wiki. |
Co-authored-by: rusty-snake <[email protected]>
Co-authored-by: rusty-snake <[email protected]>
Should I add |
@rusty-snake / others, can we have a look-over and see what really needs to be done to merge this PR? I'm somewhat partial to the comments I left above disable-xdg and disable-shell, but if I must, I can remove them. |
Merged, thanks! |
My first profile, please review and squash the commits 😁
In particular, I don't really understand what
ipc-namespace
is for, but the template says it's for CLI-only apps, so I've left it commented out.Also unclear on the private-* and dbus stuff.