Skip to content

Separate test installation directories and environments #915

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

Merged
merged 15 commits into from
Jul 14, 2014

Conversation

hamiltont
Copy link
Contributor

This PR introduces the potential to install items into per-test directories using the new flag --install-strategy (unified | pertest). Adding this necessitated a consistent way to reference the environment, so this PR also creates an environment variable FWROOT that is propagated to all subprocesses/subshells (a number of setup.py files already create a similar variable, this just formalizes it into the framework). A similar variable IROOT tells each subshell or subprocess where its installs directory is.

Propagating these core variables fwroot/iroot allows each test to declare it's own environment relative to these variables. For now, I've placed environment declaring code in <test-dir>/bash_profile.sh, but there's likely a better name. See go/bash_profile.sh for an example of the go test declaring it's own environment using these variables.

This is entirely backwards compatible - unless a test overrides the default environment with it's own bash_profile.sh, the default environment is what is used. This also removes the need to copy over top of ~/.profile, ~/.bash_profile, ~/.bashrc, as this loads the default environment itself and don't rely on bash to load it automatically.

This is all based on #900, which can be a bit confusing. To see a clean comparison of what this adds, please visit this comparison. I based it on #900 because it was a lot easier

This PR resulted from discussion on #899. It should also help to close #448 if merged

This is not fully tested, just putting it out for comment before I spend time testing. I've run enough tests to know it works generally, but it likely needs some cleanup. Also, I've dropped the --install-software flag because it was redundant with --install

@msmith-techempower
Copy link
Member

Question/thought: could we set up a trivial proxy on localhost and route requests through it? This would improve our test purposes for new-users who are going to go through every test on the first go where there is overlapped downloading. It could be part of the environment setup.

@msmith-techempower
Copy link
Member

Testing question: must my tester(s) first pull #900 and then merge this PR, or is this a separate PR that has all the changes from #900 already (read: can simply pull this PR into a new branch off master and test)?

@hamiltont
Copy link
Contributor Author

The latter, this already includes all the commits from 900. The proxy idea
is nice

hamiltont added 10 commits July 8, 2014 16:04
This commit is my proof-of concept, only the 'go' test works right now.
You can look at go/ to see the changes required on a per-framework
basis - essentially I've modified go/setup.py to move the declaration
of the environment into go/bash_profile.sh

Each framework now gets a personal <framework>/installs directory, and
can declare any needed environment variables in
<framework>/bash_profile.sh, such as modifications to the PATH.

The FwBm code now provides an FWROOT environment variable to
*all* shell scripts (instead of ~/Frameworks being assumed).
Also, an IROOT environment variable is provided to any framework-specific
files (e.g. install.sh and bash_profile.sh) for the location of the install directory
for that framework. See go/bash_profile.sh to see me using both IROOT and
FWROOT.

By using IROOT, the strategy for installation is still controlled by python code and
the frameworks just reference IROOT and FWROOT in their scripts. These
variables are a nice step towards solving TechEmpower#448

The new function replace_environ in setup_util.py goes a long way towards
addressing TechEmpower#899 - as you can see I've removed all the changes to the ~/.bash_profile
from prerequisites.sh (and this commit serves as proof that everything still works) and
most of the changes from config/benchmark_profile
- adds an '--install-strategy' option
- framework_test properly sets the install location before calling setup.py
- removes '--install-software' option (checks for '--install' instead)

Still proof-of-concept, only 'go' works!
@hamiltont
Copy link
Contributor Author

I have tested this now with a few frameworks, success. I have not done a 100% test of everything.

I've moved the pertest installation root to FWROOT/installs/pertest/<test-name> so that it's underneath the normal installs/ directory. It was annoying to chase down all those different installs folders underneath each individual test...

Just to be clear, this PR adds

  1. separating install directories
  2. the core features needed to setup a test-specific environment via <test_name>/bash_profile.sh.

This PR does not provide a bash_profile.sh for each test. If a test does not have a bash_profile, then it gets the environment that is setup in config/benchmark_config, and as far as that test is concerned nothing has changed. If a test wants to ensure it has it's own isolated environment, it can use the "new" method of bash_profile.sh

@hamiltont
Copy link
Contributor Author

This wasn't fully rebased onto master yet, now it is

@methane
Copy link
Contributor

methane commented Jul 10, 2014

I'm +1 on separating environment variables. But -1 on install all softwares per tests.
For example, virtualenv is commonly used for lightweight separating Python environment.
Buiding nginx many times just for reverse proxy is bad idea.
php have many frameworks and libraries separated by default.

Can I use pertest environment variable without pertest installation?

@methane
Copy link
Contributor

methane commented Jul 10, 2014

I'm sorry. I misunderstood.
<test>/bash_profile.sh is under <test>, not under installs/<test>.

def replace_environ(config=None, root=None, print_result=False, command='true'):
# Clean up our current environment, preserving some important items
mini_environ = os.environ.copy()
mini_environ.clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

This 2 lines means mini_environ = {} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha ha, yea....

I'll clean it up when I get a chance. I guess I didn't know at the time that env was just a dict, thought it might be some fancy data structure that I needed to copy and clear :-P

@hamiltont
Copy link
Contributor Author

Can I use pertest environment variable without pertest installation?

Just to make it explicit for anyone listening in, yes you can

@hamiltont
Copy link
Contributor Author

@methane just applied those fixes, thanks!

@hamiltont
Copy link
Contributor Author

Could someone confirm that the following is expected:

When I run a test, I get two folders under results/, one called latest and one named for the current time. Both of these folders contain a results.json file, but only latest/results.json contains the raw data. The timestamp\results.json appears to have no raw data. Is this normal!? If not, we need to examine this PR closely to see what is causing this

@methane
Copy link
Contributor

methane commented Jul 11, 2014

@hamiltont Could you try #893 ?

@hamiltont
Copy link
Contributor Author

@methane tested, #893 fixes the unexpected operation, so it's not a problem with this PR. Thanks!

@msmith-techempower
Copy link
Member

@hamiltont I am confirming that what you see is expected (though, probably not optimal).

latest contains the last benchmark run data. If you ran a test against gemini and then a test against undertow, you should only see the undertow data in latest/results.json.

In order to get that [timestamp]/results.json, you have to execute the parse directive. This is how we come up with the entire "Round X" results file.

@idlewan
Copy link
Contributor

idlewan commented Jul 24, 2014

Looking at the per-test install directory (https://travis-ci.org/hamiltont/FrameworkBenchmarks/jobs/30719688), something feels not right:

Searching for mongrel2
Installing webserver: mongrel2 in $FWROOT/installs/pertest/nawak/nawak/nawak/nimrod
Searching for zeromq
Installing webserver: zeromq in $FWROOT/installs/pertest/nawak/nawak/nawak/nimrod

It seems that the scripts don't go back to $FWROOT/installs/pertest/thenameofthetestedframework between installing the dependencies. For example, with nawak you have:

# nawak/install.sh
fw_depends nawak nimrod mongrel2

which probably means that nawak installs first, then nimrod, then mongrel2.

# toolset/setup/linux/frameworks/nawak.sh
[...]
git clone git://github.com/idlewan/nawak.git nawak/nawak
cd nawak/nawak
[...]

(there is no need for a subdirectory now, so I should change nawak/nawak to nawak anyway)
And then with nimrod you've got:

# toolset/setup/linux/languages/nimrod.sh
[...]
cd nimrod
[...]

which results in nimrod installed under the nawak git directory, and mongrel2 + zeromq installed under the nimrod directory. We might need to cd back up somewhere, to change

$FWROOT/installs/pertest/nawak/nawak/nawak/nimrod/mongrel2

into:

$FWROOT/installs/pertest/nawak/mongrel2

@hamiltont
Copy link
Contributor Author

@idlewan Good catch. The fix is simple, I'll put in a pr in a bit. If you have the experience, perhaps you could modify the mongrel2.sh and zeromq.sh files so they are installed into prefix directories instead of onto the host system?

@idlewan
Copy link
Contributor

idlewan commented Jul 24, 2014

Thanks for the fix.

I'm not sure it's strictly necessary to enforce local installation for mongrel2 and zeromq, at least not until some other framework use them too. We already install globally lots of things with apt-get, is there a need not to install anything globally?

@hamiltont
Copy link
Contributor Author

Local install is 1) to be polite for users running TFB on their main
computer and 2) to avoid future conflicts. I know a lot is currently
installed globally, but the more we can remove this the better IMO

@idlewan
Copy link
Contributor

idlewan commented Jul 24, 2014

I see.
I guess that would mean fiddling with prefix=..., C_INCLUDE_PATH and LD_LIBRARY_PATH, but it should be doable. It would still need the global dependencies currently installed with apt, though (libsqlite3-dev, libsqlite3, uuid-runtime).

@hamiltont
Copy link
Contributor Author

The global stuff is probably never going to be changed from normal apt-get,
it's a pretty small set of reasonably generic software

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.

Directory structure re-organization
5 participants