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

Shell scripts compatibility enhancements #11

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

javornikolov
Copy link
Contributor

Here are some changes in attempt to make the setup shell scripts more portable (in particular - compatible with Ubuntu 13.10).

  • Use . instead of source to load vars.sh. Seems source is not POSIX compatible. (In particular - it's not part of dash shell which is the default shell mapped to /bin/bash in recent versions of Debian and Ubuntu). (Another alternative would be to explicitly require some other shell like bash or zsh instead of sh)
  • Parameterize some parts of the variables: to allow easier override. (E.g. - my vbox dir is located elsewhere)
  • Add some up-front checks of isos and boxes dir existence to avoid having partially configured vbox VMs.

@casr
Copy link
Member

casr commented Jan 7, 2014

Wow! I've got a pretty busy week ahead but I'll try and look this over on the weekend.

Thanks for the contribution!

@casr
Copy link
Member

casr commented Jan 13, 2014

I think from reviewing some of the commits here and some of the other forks it would be worth thinking about a different command structure. I don't want to overburden the repo with loads of small script files when it might be simpler to have a single entry point and pass command line parameters (which could override the defaults in vars.sh in the common cases). I'll open a separate issue for that.

On other fronts, I'm all for having a more cross-platform script so thanks for making me consider those cases more.
Automatic creation of the output directories is a nice addition too which I had glossed over when I threw this together initially :) . I'll try and integrate those changes ASAP.

casr added a commit that referenced this pull request Jan 13, 2014
@casr
Copy link
Member

casr commented Jan 13, 2014

(Side note: I like commit messages to be formatted like git manual guidelines with a short < 50 character first line message. So I've gone ahead and reworded the commits)

Cheers for the PR!

@javornikolov
Copy link
Contributor Author

OK, cool.

994f298 seems to go against the intention of 3b7dde4 but I think it the overall idea should remain.

Yes, the commit is opposite of the other's intention. The second change was with the idea be able to run scripts from other directory with a local customized vars.sh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants