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

WIP: whipper-docker.sh is an executable script to easily launch whipper's docker #426

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

hydrian
Copy link

@hydrian hydrian commented Dec 2, 2019

A bash script to easily run the docker image from the CLI.

Copy link
Member

@Freso Freso left a 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…:

  1. 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. Use sh instead and let individual distributions determine what their sh should be. (E.g., IIRC Ubuntu points sh to dash.)
  2. 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 to whipper.

@hydrian
Copy link
Author

hydrian commented Dec 2, 2019

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.

@Freso
Copy link
Member

Freso commented Dec 15, 2019

Bash is very common on most *nix platforms.

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, dash e.g. runs with a lower memory footprint than bash which could be a concern if anyone should want to run whipper on an embedded device or similar hardware constrained environment.)

Seems like Joe fixed this part up though.

I didn't hardcode locations.

WRAPPER_CONFIG_FILE="${HOME}/.config/whipper_wrapper" is hardcoding a location, and so is PERSONAL_CONF_DIR="${HOME}/.config/whipper".

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 whipper.sh which could just as well be a Python virtuelenv wrapper (which might be well be a significantly smaller overhead than using Docker) or for a number of other wrappers. Should we ship wrapper scripts for all possible virtualisation/containerisation environments whipper might run in? I’m not necessarily opposed to included such a script, but I’m also not currently convinced of the utility of doing so… Especially as we’re stilling cleaning out bits and pieces of cruft from the morituri legacy. :)

@MerlijnWajer
Copy link
Collaborator

I also think that the name whipper.sh is confusing - it makes me think it's basic shell script with no dependencies to run whipper on your machine. It doesn't make me think it needs the whole docker kitchen sink to run whipper. ;)

@hydrian
Copy link
Author

hydrian commented Dec 17, 2019

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.

@hydrian
Copy link
Author

hydrian commented Dec 17, 2019

I was thinking about renaming to whipper-docker.sh. This way there isn't any confusion about what it used for.

@JoeLametta JoeLametta changed the title whipper.sh is an executable script to easily launch whipper's docker WIP: whipper.sh is an executable script to easily launch whipper's docker Dec 18, 2019
@hydrian
Copy link
Author

hydrian commented Dec 18, 2019

I'm also working on a documentation file for the whipper-docker.sh script.

hydrian and others added 8 commits January 3, 2020 14:55
…image

Signed-off-by: Ben Tyger <ben.tyger@tygerclan.net>
Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
Signed-off-by: hydrian <ben.tyger@tygerclan.net>
Signed-off-by: hydrian <ben.tyger@tygerclan.net>
Signed-off-by: hydrian <ben.tyger@tygerclan.net>
more sense for documentation

Signed-off-by: hydrian <ben.tyger@tygerclan.net>
Signed-off-by: hydrian <ben.tyger@tygerclan.net>
Signed-off-by: hydrian <ben.tyger@tygerclan.net>
@JoeLametta
Copy link
Collaborator

Fixed wrong Signed-off-by value + rebased on develop.

@JoeLametta JoeLametta changed the title WIP: whipper.sh is an executable script to easily launch whipper's docker WIP: whipper-docker.sh is an executable script to easily launch whipper's docker Feb 22, 2020
@JoeLametta
Copy link
Collaborator

@Freso @MerlijnWajer Are we interested in merging this pull request?

@MerlijnWajer
Copy link
Collaborator

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.

@JoeLametta
Copy link
Collaborator

@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

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.

4 participants