-
Notifications
You must be signed in to change notification settings - Fork 91
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
WIP: whipper-docker.sh is an executable script to easily launch whipper's docker #426
base: develop
Are you sure you want to change the base?
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.
This script seems too personalised and hardcoded for adoption into whipper’s codebase as it stands now. For some reason GitHub isn’t allowing me to comment on individual lines, so…:
- Don’t call
bash
(and by extension, avoid bashisms). Some systems may not have bash installed and if we as upstream want to sanction such a script, it should be as widely compatible as possible. Usesh
instead and let individual distributions determine what theirsh
should be. (E.g., IIRC Ubuntu pointssh
todash
.) - Don’t hardcode locations (e.g.,
CD_DEVICE
,OUTPUT_DIR
,PERSONAL_CONF_DIR
). It’s fine to determine defaults, but allow users to override them. Especially when these in turn may get overridden by options passed towhipper
.
Bash is very common on most *nix platforms. Almost all Linux platforms default to bash. If it is not the default, bash is usually installed. Many of the non-Linux *nixes can have it installed. Also, I'm not sure how portable whipper is to non-Linux *nixes because of some of whipper's package dependencies. I'm trying to see what the issue with bash. I could convert it to standard POSIX sh. This script is very simplisitic but handy. I didn't hardcode locations. All I did was set defaults. You can add a config file (~/.config/whipper_wrapper) to override any of those variables. I could probably add a whipper.txt for showing how to override the variables without having the user need to grok the script. |
I never said it wasn’t, I simply said to not use it. It may be installed on 99.9% of machines, but if we can make some simple changes and make it run on the last .1%, let’s do it. (Also, Seems like Joe fixed this part up though.
You also didn’t respond to what happens if a user uses any of the command-line flags for whipper to change things such as output directory (which may be fine and just me not being familiar with using Docker). Lastly, I’m not convinced about the value of this. Esp. named |
I also think that the name |
I agree with you, @MerlijnWajer . The script name does need to be renamed. The way it is named now will cause confusion since it is only used for docker execution and none of the other execution methods. |
I was thinking about renaming to whipper-docker.sh. This way there isn't any confusion about what it used for. |
I'm also working on a documentation file for the whipper-docker.sh script. |
17f0371
to
6d462ec
Compare
…image Signed-off-by: Ben Tyger <[email protected]>
Signed-off-by: JoeLametta <[email protected]>
Signed-off-by: hydrian <[email protected]>
Signed-off-by: hydrian <[email protected]>
Signed-off-by: hydrian <[email protected]>
more sense for documentation Signed-off-by: hydrian <[email protected]>
Signed-off-by: hydrian <[email protected]>
Signed-off-by: hydrian <[email protected]>
Fixed wrong |
@Freso @MerlijnWajer Are we interested in merging this pull request? |
I mean, it looks good and clean to me. I wouldn't recommend this as the defacto way to use whipper, but we can support it in some contrib manner I think. |
@hydrian Keep in mind that a user may have multiple disk drives connected to the computer and wish to make more than a single one available to the Docker container |
A bash script to easily run the docker image from the CLI.