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

Make ROOT_DATA_PATH configurable #5143

Closed
mariospr opened this Issue Jun 8, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@mariospr
Contributor

mariospr commented Jun 8, 2016

Summary

While trying to bundle KA Lite as a flatpak bundle I realized that setting the KALITE_DIR variable to point to the alternative root directory (/app/share/kalite, for the case of flatpak) was not enough, since kalite start would fail with an error on startup, since it would still look into /usr/share/kalite when copying the static media files.

By looking into kalite/init.py, it seems the problem is that the ROOT_DATA_PATH is always using python's sys.prefix to construct the data path, which will point to the non-existent /usr/share/kalite in this particular case.

So, even though I agree one option to prevent this could be to avoid doing that copy of static media, I think it would be nice if we could construct the ROOT_DATA_PATH variable considering the value of the KALITE_DIR variable, if it exists.

Branch or installer method

I'm using a development environment setup on top of my Fedora 23 machine using virtualenv and this instructions, with the 0.16.x branch.

Traceback or relevant snippet from server.log

On first run of kalite start with KA lite installed in a non-standard location (/app/share/kalite for flatpak), it will fail with this error:

[...]
[INFO] [2016-06-08 15:23:40,480] root: Copying static media...
Traceback (most recent call last):
  File "/app/share/kalite/python-packages/django/core/management/base.py", line 224, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/app/share/kalite/python-packages/django/core/management/base.py", line 263, in execute
    output = self.handle(*args, **options)
  File "/app/lib/python2.7/dist-packages/kalite/distributed/management/commands/initialize_kalite.py", line 42, in handle
    self.setup_server_if_needed()
  File "/app/lib/python2.7/dist-packages/kalite/distributed/management/commands/initialize_kalite.py", line 36, in setup_server_if_needed
    call_command("setup", interactive=False)
  File "/app/share/kalite/python-packages/django/core/management/__init__.py", line 161, in call_command
    return klass.execute(*args, **defaults)
  File "/app/share/kalite/python-packages/django/core/management/base.py", line 263, in execute
    output = self.handle(*args, **options)
  File "/app/lib/python2.7/dist-packages/kalite/distributed/management/commands/setup.py", line 494, in handle
    clear=True)
  File "/app/share/kalite/python-packages/django/core/management/__init__.py", line 161, in call_command
    return klass.execute(*args, **defaults)
  File "/app/share/kalite/python-packages/django/core/management/base.py", line 263, in execute
    output = self.handle(*args, **options)
  File "/app/share/kalite/python-packages/django/core/management/base.py", line 393, in handle
    return self.handle_noargs(**options)
  File "/app/share/kalite/python-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 164, in handle_noargs
    collected = self.collect()
  File "/app/share/kalite/python-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 105, in collect
    for path, storage in finder.list(self.ignore_patterns):
  File "/app/share/kalite/python-packages/django/contrib/staticfiles/finders.py", line 106, in list
    for path in utils.get_files(storage, ignore_patterns):
  File "/app/share/kalite/python-packages/django/contrib/staticfiles/utils.py", line 25, in get_files
    directories, files = storage.listdir(location)
  File "/app/share/kalite/python-packages/django/core/files/storage.py", line 248, in listdir
    for entry in os.listdir(path):
OSError: [Errno 2] No such file or directory: '/usr/share/kalite/static-libraries'

How to reproduce

I reproduce this after bundling the KA Lite server inside a flatpak but I believe an alternative reliable way to reproduce it should be:

  1. Install KA Lite to a non-standard location (other than <sys.prefix>/share/kalite
  2. Make sure KA Lite has not been run before, or delete previous data in CONTENT_ROOT:
  $ rm -rf ~/.kalite
  1. Run kalite start --foreground for the first time and observe the error on command line
  2. Connect to the web UI from localhost:8008 and observe how several UI assets are not loaded (and multiple 404 errors on the terminal)

Real-life consequences

Probably not a life-or-death thing, but I still believe it would be a nice thing to have as it makes easier to run KA Lite in certain non-standard environments, such as inside of a flatpak bundle

mariospr added a commit to mariospr/ka-lite that referenced this issue Jun 8, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
Otherwise, kalite start will fail to find the static media files that
it will try to copy on the first run, causing assets to be unresolvable
when loading the web UI.

learningequality#5143

mariospr added a commit to mariospr/ka-lite that referenced this issue Jun 8, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
Otherwise, kalite start will fail to find the static media files that
it will try to copy on the first run, causing assets to be unresolvable
when loading the web UI.

learningequality#5143

mariospr added a commit to mariospr/ka-lite that referenced this issue Jun 8, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
Otherwise, kalite start will fail to find the static media files that
it will try to copy on the first run, causing assets to be unresolvable
when loading the web UI.

learningequality#5143
@mariospr

This comment has been minimized.

Show comment
Hide comment
@mariospr

mariospr Jun 8, 2016

Contributor

Just pushed a pull request which I hope will be fine, although I'm unsure in how to add a test for it.

Any comment/help welcome. Thanks!

Contributor

mariospr commented Jun 8, 2016

Just pushed a pull request which I hope will be fine, although I'm unsure in how to add a test for it.

Any comment/help welcome. Thanks!

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Jun 8, 2016

Member

@mariospr the user data directory KALITE_DIR and sys.prefix-based directory ROOT_DATA_PATH are two different things. While the latter would be okay to have configurable, it does not make sense to have it respect the KALITE_DIR as it can be set while still wanting access to the system-wide directory.

I would propose that you fix it by introducing a separate KALITE_SYS_PREFIX setting.

Member

benjaoming commented Jun 8, 2016

@mariospr the user data directory KALITE_DIR and sys.prefix-based directory ROOT_DATA_PATH are two different things. While the latter would be okay to have configurable, it does not make sense to have it respect the KALITE_DIR as it can be set while still wanting access to the system-wide directory.

I would propose that you fix it by introducing a separate KALITE_SYS_PREFIX setting.

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Jun 8, 2016

Member

Thanks for opening up the issue and explaining the motivation btw :)

Member

benjaoming commented Jun 8, 2016

Thanks for opening up the issue and explaining the motivation btw :)

@mariospr

This comment has been minimized.

Show comment
Hide comment
@mariospr

mariospr Jun 8, 2016

Contributor

Ah! I knew I was missing something... that (a new variable) was actually my initial approach, but today I came across this again and thought it would make sense to make it respect KALITE_DIR.

I'll try this new idea now (already renamed the issue). Thanks!

Contributor

mariospr commented Jun 8, 2016

Ah! I knew I was missing something... that (a new variable) was actually my initial approach, but today I came across this again and thought it would make sense to make it respect KALITE_DIR.

I'll try this new idea now (already renamed the issue). Thanks!

@mariospr mariospr changed the title from Consider KALITE_DIR env variable when setting ROOT_DATA_PATH to Make ROOT_DATA_PATH configurable Jun 8, 2016

mariospr added a commit to mariospr/ka-lite that referenced this issue Jun 8, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
This will prevent `kalite start` from failing to find the static media files
that it will try to copy on the first run if the system prefix is not the
standard one, which would cause assets to be unresolvable when loading.

learningequality#5143
@mariospr

This comment has been minimized.

Show comment
Hide comment
@mariospr

mariospr Jun 8, 2016

Contributor

On a second thought, after looking into this closer and asking on IRC to @benjaoming, it is unclear to me that splitting it to a new environment variable is the right thing to do, since KALITE_SYS_PREFIX/share/kalite should probably match KALITE_DIR anyway.

Talking to @benjaoming on IRC it seems the source of the misunderstanding could come from confusing the KALITE_DIR variable, which should point to the root of the installation (under the system prefix) with the KALITE_HOME variable, which should point to where the user data files are stored.

I have prepared an alternative PR locally introducing the new variable, but before changing the PR I'd appreciate some confirmation, since it might be that the first approach was right after all? :)

Contributor

mariospr commented Jun 8, 2016

On a second thought, after looking into this closer and asking on IRC to @benjaoming, it is unclear to me that splitting it to a new environment variable is the right thing to do, since KALITE_SYS_PREFIX/share/kalite should probably match KALITE_DIR anyway.

Talking to @benjaoming on IRC it seems the source of the misunderstanding could come from confusing the KALITE_DIR variable, which should point to the root of the installation (under the system prefix) with the KALITE_HOME variable, which should point to where the user data files are stored.

I have prepared an alternative PR locally introducing the new variable, but before changing the PR I'd appreciate some confirmation, since it might be that the first approach was right after all? :)

mariospr added a commit to mariospr/ka-lite that referenced this issue Jun 9, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
This will prevent `kalite start` from failing to find the static media files
that it will try to copy on the first run if the system prefix is not the
standard one, which would cause assets to be unresolvable when loading.

learningequality#5143
@mariospr

This comment has been minimized.

Show comment
Hide comment
@mariospr

mariospr Jun 9, 2016

Contributor

JFTR, we agreed on the PR to go with the initial approach, now pending on final review. Thanks!

Contributor

mariospr commented Jun 9, 2016

JFTR, we agreed on the PR to go with the initial approach, now pending on final review. Thanks!

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Jun 9, 2016

Member

Fixed in #5144

Member

benjaoming commented Jun 9, 2016

Fixed in #5144

@benjaoming benjaoming closed this Jun 9, 2016

mariospr added a commit to mariospr/ka-lite-source that referenced this issue Jul 11, 2016

Make ROOT_DATA_PATH consider the KALITE_DIR environment variable
This will prevent `kalite start` from failing to find the static media files
that it will try to copy on the first run if the system prefix is not the
standard one, which would cause assets to be unresolvable when loading.

learningequality/ka-lite#5143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment