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.

Sign up for GitHub

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

Jump to bottom

Add server side CoreConfig object #6991

Merged
merged 11 commits into from Aug 14, 2019
Merged

Add server side CoreConfig object #6991

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2b64353
Fix deprecation warning
vidartf Aug 12, 2019
6a3c034
Add a CoreConfig object to python
vidartf Aug 12, 2019
29a40e0
fix clean
vidartf Aug 12, 2019
849244d
Expose current info on coreconfig
vidartf Aug 12, 2019
64ba1ca
Avoid camelCase in Python
vidartf Aug 12, 2019
b937f18
Fix CoreConfig signleton location
vidartf Aug 12, 2019
3c6a864
More fixes for CoreConfig
vidartf Aug 12, 2019
87b8986
Merge branch 'master' into coreconfig
vidartf Aug 12, 2019
a99899e
Lint COTRIBUTING.md
vidartf Aug 12, 2019
900b847
Update coreconfig docstrings
vidartf Aug 12, 2019
853a5f5
Tweak and test
vidartf Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions CONTRIBUTING.md
View file
Open in desktop
Expand Up @@ -44,13 +44,14 @@ a keyboard shortcut or automatically on save.
## Submitting a Pull Request Contribution

Generally, an issue should be opened describing a piece of proposed work and the
issues it solves before a pull request is opened.
issues it solves before a pull request is opened.

### Issue Management
Opening an issue lets community members participate in the design discussion,
makes others aware of work being done, and sets the stage for a fruitful community
interaction. A pull request should reference the issue it is addressing. Once the
pull request is merged, the issue related to it will also be closed. If there is

Opening an issue lets community members participate in the design discussion,
makes others aware of work being done, and sets the stage for a fruitful community
interaction. A pull request should reference the issue it is addressing. Once the
pull request is merged, the issue related to it will also be closed. If there is
additional discussion around implemementation the issue may be re-opened. Once 30 days
have passed with no additional discussion, the [lock bot](https://github.com/apps/lock) will lock the issue. If
additional discussion is desired, or if the pull request doesn't fully address the
Expand Down
92 changes: 43 additions & 49 deletions jupyterlab/commands.py
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@

from .semver import Range, gte, lt, lte, gt, make_semver
from .jlpmapp import YARN_PATH, HERE
from .coreconfig import CoreConfig


# The regex for expecting the webpack output.
Expand Down Expand Up @@ -283,7 +284,7 @@ def watch_dev(logger=None):
return package_procs + [wp_proc]


def watch(app_dir=None, logger=None):
def watch(app_dir=None, logger=None, core_config=None):
"""Watch the application.

Parameters
Expand All @@ -299,11 +300,11 @@ def watch(app_dir=None, logger=None):
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.watch()


def install_extension(extension, app_dir=None, logger=None):
def install_extension(extension, app_dir=None, logger=None, core_config=None):
"""Install an extension package into JupyterLab.

The extension is first validated.
Expand All @@ -312,24 +313,24 @@ def install_extension(extension, app_dir=None, logger=None):
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.install_extension(extension)


def uninstall_extension(name=None, app_dir=None, logger=None, all_=False):
def uninstall_extension(name=None, app_dir=None, logger=None, all_=False, core_config=None):
"""Uninstall an extension by name or path.

Returns `True` if a rebuild is recommended, `False` otherwise.
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
if all_ is True:
return handler.uninstall_all_extensions()
return handler.uninstall_extension(name)


def update_extension(name=None, all_=False, app_dir=None, logger=None):
def update_extension(name=None, all_=False, app_dir=None, logger=None, core_config=None):
"""Update an extension by name, or all extensions.

Either `name` must be given as a string, or `all_` must be `True`.
Expand All @@ -339,7 +340,7 @@ def update_extension(name=None, all_=False, app_dir=None, logger=None):
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
if all_ is True:
return handler.update_all_extensions()
return handler.update_extension(name)
Expand All @@ -363,96 +364,96 @@ def clean(app_dir=None, logger=None):

def build(app_dir=None, name=None, version=None, static_url=None,
logger=None, command='build:prod', kill_event=None,
clean_staging=False):
clean_staging=False, core_config=None):
"""Build the JupyterLab application.
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger, kill_event=kill_event)
handler = _AppHandler(app_dir, logger, kill_event=kill_event, core_config=core_config)
return handler.build(name=name, version=version, static_url=static_url,
command=command, clean_staging=clean_staging)


def get_app_info(app_dir=None, logger=None):
def get_app_info(app_dir=None, logger=None, core_config=None):
"""Get a dictionary of information about the app.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.info


def enable_extension(extension, app_dir=None, logger=None):
def enable_extension(extension, app_dir=None, logger=None, core_config=None):
"""Enable a JupyterLab extension.

Returns `True` if a rebuild is recommended, `False` otherwise.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.toggle_extension(extension, False)


def disable_extension(extension, app_dir=None, logger=None):
def disable_extension(extension, app_dir=None, logger=None, core_config=None):
"""Disable a JupyterLab package.

Returns `True` if a rebuild is recommended, `False` otherwise.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.toggle_extension(extension, True)


def check_extension(extension, app_dir=None, installed=False, logger=None):
def check_extension(extension, app_dir=None, installed=False, logger=None, core_config=None):
"""Check if a JupyterLab extension is enabled or disabled.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.check_extension(extension, installed)


def build_check(app_dir=None, logger=None):
def build_check(app_dir=None, logger=None, core_config=None):
"""Determine whether JupyterLab should be built.

Returns a list of messages.
"""
logger = _ensure_logger(logger)
_node_check(logger)
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.build_check()


def list_extensions(app_dir=None, logger=None):
def list_extensions(app_dir=None, logger=None, core_config=None):
"""List the extensions.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.list_extensions()


def link_package(path, app_dir=None, logger=None):
def link_package(path, app_dir=None, logger=None, core_config=None):
"""Link a package against the JupyterLab build.

Returns `True` if a rebuild is recommended, `False` otherwise.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.link_package(path)


def unlink_package(package, app_dir=None, logger=None):
def unlink_package(package, app_dir=None, logger=None, core_config=None):
"""Unlink a package from JupyterLab by path or name.

Returns `True` if a rebuild is recommended, `False` otherwise.
"""
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.unlink_package(package)


def get_app_version(app_dir=None):
def get_app_version(app_dir=None, core_config=None):
"""Get the application version."""
app_dir = app_dir or get_app_dir()
handler = _AppHandler(app_dir)
handler = _AppHandler(app_dir, core_config=core_config)
return handler.info['version']


def get_latest_compatible_package_versions(names, app_dir=None, logger=None):
def get_latest_compatible_package_versions(names, app_dir=None, logger=None, core_config=None):
"""Get the latest compatible version of a list of packages.
"""
app_dir = app_dir or get_app_dir()
handler = _AppHandler(app_dir, logger)
handler = _AppHandler(app_dir, logger, core_config=core_config)
return handler.latest_compatible_package_versions(names)


Expand All @@ -476,12 +477,13 @@ def read_package(target):

class _AppHandler(object):

def __init__(self, app_dir, logger=None, kill_event=None):
def __init__(self, app_dir, logger=None, kill_event=None, core_config=None):
"""Create a new _AppHandler object
"""
self.app_dir = app_dir or get_app_dir()
self.sys_dir = get_app_dir()
self.logger = _ensure_logger(logger)
self.core_data = (core_config or CoreConfig()).data
Copy link
Member

Choose a reason for hiding this comment

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

@vidartf I mean that we are using the data here in the app handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't the docstring of data cover this use?

self.info = self._get_app_info()
self.kill_event = kill_event or Event()
# TODO: Make this configurable
Expand Down Expand Up @@ -713,7 +715,6 @@ def uninstall_extension(self, name):
Returns `True` if a rebuild is recommended, `False` otherwise.
"""
# Allow for uninstalled core extensions.
data = self.info['core_data']
if name in self.info['core_extensions']:
config = self._read_build_config()
uninstalled = config.get('uninstalled_core_extensions', [])
Expand Down Expand Up @@ -775,7 +776,7 @@ def update_extension(self, name):
Returns `True` if a rebuild is recommended, `False` otherwise.
"""
if name not in self.info['extensions']:
self.logger.warn('No labextension named "%s" installed' % name)
self.logger.warning('No labextension named "%s" installed' % name)
return False
return self._update_extension(name)

Expand Down Expand Up @@ -815,8 +816,8 @@ def link_package(self, path):
return self.install_extension(path)

# Warn that it is a linked package.
self.logger.warn('Installing %s as a linked package:', path)
[self.logger.warn(m) for m in messages]
self.logger.warning('Installing %s as a linked package:', path)
[self.logger.warning(m) for m in messages]

# Add to metadata.
config = self._read_build_config()
Expand Down Expand Up @@ -938,7 +939,7 @@ def _get_app_info(self):
"""

info = dict()
info['core_data'] = core_data = _get_core_data()
info['core_data'] = core_data = self.core_data
info['extensions'] = extensions = self._get_extensions(core_data)
page_config = self._read_page_config()
info['disabled'] = page_config.get('disabledExtensions', [])
Expand All @@ -963,7 +964,8 @@ def _get_app_info(self):
info['sys_dir'] = self.sys_dir
info['app_dir'] = self.app_dir

info['core_extensions'] = core_extensions = _get_core_extensions()
info['core_extensions'] = core_extensions = _get_core_extensions(
self.core_data)

disabled_core = []
for key in core_extensions:
Expand Down Expand Up @@ -1390,9 +1392,8 @@ def _install_extension(self, extension, tempdir):
raise ValueError(msg % (extension, '\n'.join(messages)))

# Verify package compatibility.
core_data = _get_core_data()
deps = data.get('dependencies', dict())
errors = _validate_compatibility(extension, deps, core_data)
errors = _validate_compatibility(extension, deps, self.core_data)
if errors:
msg = _format_compatibility_errors(
data['name'], data['version'], errors
Expand Down Expand Up @@ -1628,7 +1629,7 @@ def _node_check(logger):
output = subprocess.check_output([node, 'node-version-check.js'], cwd=HERE)
logger.debug(output.decode('utf-8'))
except Exception:
data = _get_core_data()
data = CoreConfig().data
ver = data['engines']['node']
Copy link
Member

Choose a reason for hiding this comment

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

For example, we expect there to be 'engines': 'node' metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

The content of data is supposed to be a lab internal detail. As an alternative, I could call the method ._data, to underline the fact? Or ._get_data_internal() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The content of data is supposed to be a lab internal detail

C.f. its docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so your custom spin doesn't use our AppHandler, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but it only modifies data via the methods on CoreConfig. As such, the internal detail fields like engine are left untouched.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on my phone, but doesn't that metadata still need to be there then? As in, we need to document the minimum fields that need to be present?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the data is originally filled with the data from staging/package.json. This public API only allows for modifying the extensions/mimeExtensions/singletons. The data property (getter only) is meant to be used by whoever consumes the core config (internal only). Technically, since we are returning the inner data by reference, a user could modify the content of data in place. The behavior in this case is undefined (by spec), and can be considered similar in nature to monkey patching. We could try to make this harder to do by accident by doing a deepcopy on access, but I'm not sure if that is worth the hassle.

Copy link
Member

@blink1073 blink1073 Aug 14, 2019 •
edited

Choose a reason for hiding this comment

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

It feels off to me that they'd have to use our package
json directly as a starting point. I'd rather they had to provide their own metadata that met a certain spec.

msg = 'Please install nodejs %s before continuing. nodejs may be installed using conda or directly from the nodejs website.' % ver
raise ValueError(msg)
Expand Down Expand Up @@ -1728,13 +1729,6 @@ def _tarsum(input_file):
return h.hexdigest()


def _get_core_data():
"""Get the data for the app template.
"""
with open(pjoin(HERE, 'staging', 'package.json')) as fid:
return json.load(fid)


def _get_static_data(app_dir):
"""Get the data for the app static dir.
"""
Expand Down Expand Up @@ -1931,10 +1925,10 @@ def _compat_error_age(errors):
return 0


def _get_core_extensions():
def _get_core_extensions(core_data):
"""Get the core extensions.
"""
data = _get_core_data()['jupyterlab']
data = core_data['jupyterlab']
return list(data['extensions']) + list(data['mimeExtensions'])


Expand Down