run_tests without venv broken

Bug #1462922 reported by Matthias Runge
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Dashboard (Horizon)
Expired
Undecided
Unassigned

Bug Description

as stated in review of https://review.openstack.org/#/c/188225/

this patch breaks tests without using venv. This is e.g. true, when packaging the whole thing, as builders don't have the connection to the internet

For me, this is a critical regression.

Revision history for this message
Michael (krotscheck) wrote :

To continue the conversation, I repeat my comments here:

I was hoping you'd comment :). So, the goal here is to make a single call (`npm install`) bootstrap the entire development environment so that tests can be run, in a way that makes sense to both developers and packagers. That's why I have the if statement in the 'postinstall' hook, to detect the presence of two different venvs, and I would love any suggestions you have on how to detect whether to use .venv or the local environment.

Some constraints:

* Must work on infra, packager, and developer boxes (so must automagically detect where to get its dependencies from and/or whether to setup a venv).
* Must _only_ set up the environment. run_tests.sh -N actually runs tests, which we don't really want it to do in this case.
* Must not be using a deprecated way of building things: run_tests.sh is deprecated across openstack, Horizon's one of the few projects still using it. I would strongly prefer we not continue to overload something which should really go away.

In a perfect world, which I'm working towards, all the "Where do I get my dependencies from" should be resolved by npm and/or bower itself, automatically linking in appropriate sources if it detects them. So whatever steps we take, I'd like them to move us in that direction, though we'll have to put temporary shims in place as we get there.

Revision history for this message
Michael (krotscheck) wrote :

A little investigation into the difference between debian systems and redhat systems tells me that neither of them uses a consistent packaging folder structure either, so we'd have to try to derive the location of the dependencies from four different locations. It is not the responsibility of the horizon team to support brittle inconsistencies between the consumers of its code. If the packagers themselves want to present a solution, I've listed my own requirements for such a solution above.

http://anonscm.debian.org/cgit/openstack/python-xstatic-spin.git/tree/debian/patches/debianize.patch

Revision history for this message
Matthias Runge (mrunge) wrote :

I don't see, how the corrected patch doesn't work on infra.

run_test.sh -N executes tests. That's exactly, what it should do. I would even argue, it should not try to install an environment.

unfortunately, neither bower, nor npm works are a replacement for real package management tools. It might be ok to use them to download something and to place those files somewhere.

Both will get you in hells kitchen, when trying to keep a managed system up to date or when trying to remove a package.

using venv for development might be ok. but pip happily overwrites your installed files (thus effectively replacing, what has been there with higher/lower versions). Personally, I can't recommend to use it at all. It's not obvious, what's installed, it's hard to update (stuff containing security risks) etc.

Calling differences between debian and rpm packaging doesn't help us here.

Changed in horizon:
status: New → Confirmed
Revision history for this message
Matthias Runge (mrunge) wrote :

Apparently, tests are still working, but:

hard coding search paths for static data to './.venv', './.tox/py27' does not help at all.

Fonts are to be placed in fonts folder under /usr/share/fonts; that is unchanged when using xstatic, but xstatic provides an indirection layer here.

Revision history for this message
Matthias Runge (mrunge) wrote :

As one can see at https://kojipkgs.fedoraproject.org//work/tasks/13/11460013/build.log (scroll to the bottom)

there is output from run_tests.sh -N
-N does not install a venv, but uses packages installed system wide, i.e. use packaged dependencies for the test.
For packaging, that is a hard requirement.

Revision history for this message
Thomas Goirand (thomas-goirand) wrote :

Just reading this in the patch scares me:

xstaticPath = base_path + '/lib/python2.7/site-packages/xstatic/pkg/';

This is in many ways completely wrong. First, there's no reasons why a package would double guess path in the system. I could decide that my system use /opt for everything Python related, for example. But more than this, "site-packages" doesn't exist in Debian at all, in Debian, we use "dist-packages" instead. So this is very Red Hat centric.

Then, this patch assumes Python 2.7. What will happen with Python 3?!?

This is a way too many assumptions. The only thing that the dashboard should be doing is doing import xstatic.pkg.foo, then read BASE_DIR in it, and then only use that path, which it may in some ways provide to an eventual Javascript if this is really needed (though I doubt it would be, and I also think it's dangerous to do so). That's the only valid way to do things, everything else is simply wrong.

Revision history for this message
Thomas Goirand (thomas-goirand) wrote :

Gosh... more non-sense. From the same horizon/karma.conf.js, I can read:

xstaticPath + 'jquery/data/jquery.js'

bad luck. In Debian, the xstatic-jquery package has the data folder completely removed, and BASE_DIR in that package set to /usr/share/javascript/jquery. So even if we fix the issue with karma.conf.js double wrong double-guessing of path, it will still attempt to load files which will never exist on the system.

Revision history for this message
Timur Sufiev (tsufiev-x) wrote :

Matthias, is it still as Critical as before considering work being done at https://review.openstack.org/#/c/259013/ ?

Adriano (dritec)
Changed in horizon:
assignee: nobody → Adriano (dritec)
Adriano (dritec)
Changed in horizon:
assignee: Adriano (dritec) → nobody
Revision history for this message
Matthias Runge (mrunge) wrote :

Timur,
the issue is mostly about forcing users to use .venvs. There are a number of good reasons not to use them, biggest issue is: dependencies of dependencies happily update already installed packages. At the end, it tests something other than you specified.

Revision history for this message
Akihiro Motoki (amotoki) wrote :

It was reported two years ago and there is no progress so far. It is not clear if it is still a valid problem. In addition, we has dropped run_tests.sh at the beginning of Queens cycle. I would like to set the status to Incomplete and the priority to Undecided. If it is still a problem, please add more information on the current status.

Changed in horizon:
status: Confirmed → Incomplete
importance: Critical → Undecided
Revision history for this message
Launchpad Janitor (janitor) wrote :

[Expired for OpenStack Dashboard (Horizon) because there has been no activity for 60 days.]

Changed in horizon:
status: Incomplete → Expired
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.