Skip to content

Conversation

@hamiltont
Copy link
Contributor

This seems like a big one, but it's basically switching us from using "bash-through-python" for the installation to using bash directly. This only changes the server installation - I've left the client and database as is. I have tested every single dependency shell script for correctness. I have tested running a benchmark with 2-3 frameworks.

The biggest benefits:

  • The exclude and test flags apply to the installation stage. No need to install everything if all you want is nodejs
  • Readability: It's a lot easier to parse than the previous approach
  • Maintainability: I've found/fixed a few bugs already
  • Future-proofing: Individual frameworks can use fw_depends to get the language/tool/etc that is current for FrameworkBenchmarks, or they can just ignore fw_depends and use the specific version they need. Best of both worlds :-)
  • True installation stage: A lot of our setup.py scripts do some last-minute installation, which is annoying when you've waited all night to install and just want to start running. With each folder having an install.sh, we can move the installation there and fix the setup.py#start() usage to be as it was intended.

Basically this just declares a few bash functions in toolset/setup/linux/bash_functions.sh and then uses them everywhere. Each framework now has an install.sh file that declares it's dependencies (e.g. fw_depends siena play1) - if a dependency is unknown it's ignored. Each dependency has it's own bash script in the proper subfolder (e.g. frameworks/play1.sh) that is executed to install it. About 95% of these dependencies check if they have already been met before doing anything, so calling them repeatedly has no performance hit (e.g. fw_depends nginx nginx nginx just install once and then skip twice).

Bugs Fixed

  • HHVM: Was forcing trusty install regardless of ubuntu version
  • WT: was checking for installs/namak folder
  • ruby: extra single quotes were breaking rvm source
  • ruby: was trying to use rvm before sourcing ~/.profile

I've stuck to traditional sh code instead of bash code, so if we want to port this to a different OS it should be trivial (at least for this boilerplate part....not for the actual installations...)

FWIW, I'm not claiming this is a generally good solution to dependency installation, but I do think it's a step in the right direction

Logically this code is quite similar to before, it's just
using the shell directly instead of going through python.

This defines a trivial dependency system with `fw_depends`.
Where possible, I have specified inherent dependencies (e.g.
siena depends on play1), but callers should declare their
dependencies in proper order to be safe.

Bugs Fixed:
- HHVM: Was forcing trusty name on all Ubuntu versions
- WT: Was checking for installs/namak
- Ruby: Removed single quotes preventing proper source
- Ruby: Was attempting to use rvm before sourcing ~/.profile
@hamiltont
Copy link
Contributor Author

@bhauer - I'm sure you guys are wicked busy, but if there's any way to get feedback on this soon that would be much appreciated - better than me having to rebase each time someone touches any part of the installer

Copy link
Contributor

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prereqs list comes straight from installer.py#__install_server_software.

I've made two changes - it now checks if prereqs have already been installed before it does anything, and it now backs up all the ~/.bash* and ~/.profile files before it overwrites them (just in case you're running this on your normal computer instead of a VM, I'd rather not accidentally wipe out someone's customizations!)

Copy link
Contributor

Choose a reason for hiding this comment

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

+  mv ~/.bash_profile ~/.bash_profile.bak
+  mv ~/.bash_profile ~/.bash_profile.bak
+  mv ~/.bash_profile ~/.bash_profile.bak

This should be

+  mv ~/.bash_profile ~/.bash_profile.bak
+  mv ~/.profile ~/.profile.bak
+  mv ~/.bashrc ~/.bashrc.bak

@methane
Copy link
Contributor

methane commented Jul 3, 2014

Great Job!

@hamiltont
Copy link
Contributor Author

Thanks!

@methane
Copy link
Contributor

methane commented Jul 3, 2014

Why is ._dart.sh binary?

@hamiltont
Copy link
Contributor Author

Ah that's a mistake - silly editor backup. I'll pull it out in a sec...

EDIT: I've removed that file

Copy link
Contributor

Choose a reason for hiding this comment

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

Is cd in script safe?
How about cd xsp && ./autogen.sh --prefix=/usr/local

I think mono and xsp should be installed in installs directory someday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cd in a subshell is perfectly safe - it only changes the working directory for the currently executing file. Once xsp.sh completes, any cd done inside is effectively undone

This is on two lines because I'm trying to be consistent with other files, such as mono, where I do a single cd then configure make make make install. In these files, doing one cd is a lot cleaner than having cd foo && four times

Also removes accidentally added ._dart.sh
@hamiltont
Copy link
Contributor Author

PS - you'll note that a lot of the install.sh files are a bit rough - this is only because I'm being a bit lazy. For example, dart-start/install.sh:fw_depends start dart nginx dart is clearly repeating itself...but I couldn't find a way to script removing these duplicates, and I'm lazy to go through all 100 files. It's not biggie as it will just skip the second one. You'll also notice that some of the dependencies listed are nonexistent (e.g. there's not start.sh for the start dependency listed above). Again, this is just me being lazy...I couldn't easily script those out, and it's no biggie because it will just ignore start when it doesn't find a corresponding start.sh

@idlewan
Copy link
Contributor

idlewan commented Jul 3, 2014

* ldconfig was running every single install, instead of just after mongrel.
  I've placed it into mongrel2.sh for now to only run after a clean mongrel install.
  Was it supposed to run every time? 

No, it was specified in Mongrel2's install guide, so it's only for that (http://mongrel2.org/manual/book-finalch3.html).

* [BUG] Mongrel2 does not build.
  The zmc_compat.h that is used to patch the downloaded tar references a file zmq.h
   that does not exist

Mongrel2 depends on zeromq being installed before, else it cannot compile.

This is an awesome pull request.
I don't like the bash through python approach very much, I found myself commenting everything just to test a specific framework. We should also have a way of only installing dependencies for one framework, it would make things easier for testing changes and updating frameworks.

@hamiltont
Copy link
Contributor Author

For anyone interested, this change is part of my progress towards packaging all of the servers inside a linux container (docker, specifically) environment. I've already done this packaging once external to TechEmpower (via my webjuice project), but I've realized I can really easily do this inside the framework now due to some upgrades in docker.

Once I'm done, this should remove huge dependency problems, as each server will automagically run in it's own isolated environment if you choose to use docker instead of installing locally. So yea...that's where I'm headed with this crazyness

@hamiltont
Copy link
Contributor Author

@idlewan Nice, two down. Added it as a depends and tested - mongrel2 installs ok now. Thanks

PS - regarding "We should also have a way of only installing dependencies for one framework, it would make things easier for testing changes and updating frameworks." these commits now support that. The normal -test and -exclude flags are now considered when using the --install server flag, so you can do this

toolset/run-tests.py --test nodejs --install-software --install server

and it will only install nodeJS software + deps, as well as the prerequisites (those always have to be installed)

@msmith-techempower
Copy link
Member

Okay, I'm going to have our team drop what they're doing and work with you on this one, @hamiltont

Just as a note, there was something about Ubuntu12 not supporting Wt (I think; not looking back). We are using Ubuntu14 on all our environments for Round10.

@hamiltont
Copy link
Contributor Author

Sounds good - I'll be on for a few more hours today. If anyone needs a chat, ping me on freenode at #techempower-fwbm

@bhauer
Copy link
Contributor

bhauer commented Jul 3, 2014

@hamiltont Thanks so much for putting this together! It's a huge contribution and I think it will be a great assist to anyone who wants to contribute to the project in the future.

@bhauer
Copy link
Contributor

bhauer commented Jul 3, 2014

@methane Thanks for your contributions to this as well!

@aschneider-techempower
Copy link
Contributor

@hamiltont

The installer worked fine, for the most part. I'm in the process of verifying all the frameworks.

There's no error handling in your scripts - where before, we could more or less figure out how the install went by grepping for ^INSTALL ERROR, that is no longer the case with your script. If you'd be able to figure out a way to log errors in a consistent format, that'd be great. Many people suggest using trap 'echo INSTALL ERROR' ERR, but that is impossible to do in sh or dash, which brings me to my next point:

Ubuntu symlinks /bin/sh to /bin/dash, which has subtle differences from bash. It doesn't look like your scripts use any bashisms/dashisms, but to avoid confusion for future maintainers, can you please change your shebangs to /bin/dash or /bin/bash, depending on your intention?

@hamiltont
Copy link
Contributor Author

Yea, a decent error handler is a great idea - I'll toss one in. BTW, you do realize that every call to subprocess in the current codebase is currently using /bin/sh, right? There is already an inconsistency - I just thought you'd prefer sh over bash for reasons of future portability. However, now that I think about it...many of the installers use apt, and I can't think of a flavor that uses apt but doesn't come packaged with bash....so I suppose portability is a pretty weak argument anyways, and people are more likely to find bash syntax user-friendly over sh syntax. I also didn't know ERR wasn't supported by sh or dash, that's good to know!

@methane
Copy link
Contributor

methane commented Jul 3, 2014

In addition to trap, these option may help debugging.

#!/bin/bash
PS4="==> "
set -u
set -e
set -x
trap "echo failed" 0

foo="bar"
echo "Hello"

false || echo "False"
false
echo "World"

Output:

==> trap 'echo failed' 0
==> foo=bar
==> echo Hello
Hello
==> false
==> echo False
False
==> false
==> echo failed
failed

@hamiltont
Copy link
Contributor Author

Took a while to get it just right, but here's the new stuff.

Changes

  • Explicitly using /bin/bash, checkbashisms reports no issues in toolset/setup/linux scripts
  • Error handling is now done in both bash and python. More below
  • Dependencies are case-insensitive (fw_depends Dart==fw_depends dart). Also, warning messages are printed if no installer is found (e.g. fw_depends foobar)

Error Handing

fw_depends now has a decent ERR trap. Failed output now looks like this:

DEBUG:root:Running installer for nodejs
INSTALL: . ../toolset/setup/linux/bash_functions.sh && . ../nodejs/install.sh (cwd=installs)
<snip>
ERROR: nodejs.sh at line 3 - command 'return 3' exited with status: 3
ERROR: nodejs may not be installed properly 
INSTALL ERROR: CalledProcessError: Command '. ../toolset/setup/linux/bash_functions.sh && . ../nodejs/install.sh' returned non-zero exit status 1

Note the original INSTALL ERROR messages are now printed too, you can continue to use the installer.py#__install_error function if you like. This error reporting may be a bit verbose, but it does tell you exactly what happened (nodejs.sh at line 3 returned code 3 from command return 3).

Installation failed message are printed when the ${depend}.sh file returns a non-zero, or if the ERR trap was ever fired

@marko-asplund
Copy link
Contributor

From a test developer point of view, having to install all the languages, servers and frameworks is lot of wasted time.

I ended up solving this issue using somewhat different approach in pull request #911.
#911 doesn't address all the other issues described here (e.g. dependencies, compartmentalization).

@hamiltont
Copy link
Contributor Author

@marko-asplund very cool work, I'm sorry to see we duplicated our efforts.

However, do you see any benefits of your approach over mine? From where I stand, I would much rather prefer that each test declares the components it needs, and the installer then installs what is needed for that test. You've split the installation into a separate stage, which is good, but you haven't linked it to the specific tests being run, which IMO is bad. For example, if you want run nodejs and the run go, you have to update config.ini in between.

@marko-asplund
Copy link
Contributor

@hamiltont I think the merits of each approach depend on your target. Though there's some overlap, the implementations are pretty different. Your pull request clearly has more ambitious goals. IMO, the benefit of my approach, depending on the target, is that it was designed to be simple, require minimal changes and be easy to understand and merge to the upstream codebase.

My primary target was just to speed up development and testing when adding new languages, frameworks and servers. I was planning to extend the same mechanism for making test execution configurable, as well. But, as long as this issue gets addressed, I'm happy, no matter which solutions gets adopted :-)

@hamiltont
Copy link
Contributor Author

Oops - I mucked up the ERR trap a bit. This last commit fixes it, but I'll need to apply the fix to all the dependency files - I'll get to it later todayish.

@marko-asplund Sounds good to me - whatever solution works for the aims of the project is what I'm for

@hamiltont
Copy link
Contributor Author

@aschneider-techempower both those issues are now fixed

@aschneider-techempower
Copy link
Contributor

I was about to edit my comment to post that the new commit does indeed fix it, but you beat me to the punch. I'll run a verify on a bunch of tests, and if it works, I'll try to have this merged before the end of today.

Line 33-35 need changing if I'm not mistaken.

@hamiltont
Copy link
Contributor Author

Good catch, fixed it.

aschneider-techempower added a commit that referenced this pull request Jul 9, 2014
Compartmentalize install files

All the install scripts are working, but some tests may be broken.
@aschneider-techempower aschneider-techempower merged commit 0bb3773 into TechEmpower:master Jul 9, 2014
@msmith-techempower
Copy link
Member

YIS!!!!

@aschneider-techempower
Copy link
Contributor

All install steps are working, but some of the benchmarks are having trouble running. I'm not sure how much of it is related to the environment that I'm trying to run it on, so I'll try out another environment, but this way we can get started on some other PRs.

@hamiltont
Copy link
Contributor Author

Awesome. Thanks for the hard work in testing!

@msmith-techempower
Copy link
Member

No no, thank you for your hard work, @hamiltont .

@hamiltont
Copy link
Contributor Author

:)

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.

8 participants