CASSANDRA-17684 Bundle CQL.html as Python package resource #4584
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses macOS compatibility for the cqlsh HELP command by bundling CQL documentation as a Python package resource, fixing issues where /usr/share is not writable on macOS.
Changes:
- Adds
/usr/local/shareas a fallback documentation path for macOS compatibility - Bundles
CQL.htmlandCQL.cssas Python package resources for pip-installed deployments - Updates documentation path resolution logic with priority-based fallback mechanism
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pylib/setup.py | Configures package to include CQL.html/CSS as package data in cqlshlib.resources |
| pylib/cqlshlib/resources/init.py | New subpackage for bundled documentation resources |
| pylib/cqlshlib/cqlshmain.py | Updates docspath resolution with macOS path support and package resource fallback |
| pylib/cqlshlib/test/test_docspath.py | Adds comprehensive unit tests for documentation path resolution logic |
| build.xml | Adds build target to copy generated CQL docs to pylib during build process |
| .gitignore | Excludes generated CQL documentation files in pylib/resources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_returns_none_on_import_error(self): | ||
| """Should return None if importlib.resources is not available.""" | ||
| with patch.dict('sys.modules', {'importlib.resources': None}): | ||
| with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): | ||
| with patch('builtins.__import__', side_effect=ImportError): | ||
| result = _get_docs_from_package_resource() | ||
| self.assertIsNone(result) | ||
|
|
||
| def test_returns_none_when_resource_not_found(self): | ||
| """Should return None if the resource file doesn't exist.""" | ||
| from unittest.mock import MagicMock | ||
| from contextlib import contextmanager | ||
|
|
||
| @contextmanager | ||
| def mock_as_file(resource): | ||
| mock_path = MagicMock() | ||
| mock_path.exists.return_value = False | ||
| yield mock_path | ||
|
|
||
| with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): | ||
| with patch('importlib.resources.files') as mock_files: | ||
| with patch('importlib.resources.as_file', mock_as_file): | ||
| mock_files.return_value.joinpath.return_value = MagicMock() | ||
| result = _get_docs_from_package_resource() | ||
| self.assertIsNone(result) | ||
|
|
||
| def test_exception_handling(self): | ||
| """Should handle exceptions gracefully and return None.""" | ||
| with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): | ||
| with patch('importlib.resources.files', side_effect=Exception("Test error")): | ||
| result = _get_docs_from_package_resource() | ||
| self.assertIsNone(result) |
There was a problem hiding this comment.
All tests in TestGetDocsFromPackageResource mock sys.version_info to (3, 9), which means the Python 3.8 compatibility code path (lines 2170-2175 in cqlshmain.py) is never tested. Add test cases that set sys.version_info to (3, 8) to ensure the Python 3.8 compatibility path works correctly.
| package_data={ | ||
| "cqlshlib.resources": ["CQL.html", "CQL.css"], | ||
| }, | ||
| include_package_data=True, |
There was a problem hiding this comment.
The 'include_package_data=True' setting is redundant when explicitly using 'package_data'. The 'include_package_data' option is used to include files specified in MANIFEST.in, but since you're explicitly listing the files in 'package_data', this setting is unnecessary. Consider removing it to avoid potential confusion or conflicts. If you want to keep it for other files, ensure you have a MANIFEST.in file or clarify the intent.
| include_package_data=True, |
| <target name="copy-cql-docs-to-pylib" depends="generate-cql-html" | ||
| description="Copy CQL documentation to pylib resources for packaging"> | ||
| <copy todir="${pylib.resources}" failonerror="true"> | ||
| <fileset dir="doc/cql3"> | ||
| <include name="CQL.html"/> | ||
| <include name="CQL.css"/> | ||
| </fileset> | ||
| </copy> | ||
| </target> |
There was a problem hiding this comment.
The copy task should ensure the target directory exists before attempting to copy files. While the directory should exist due to the init.py file in version control, it's a best practice to add mkdir to handle edge cases or if the directory structure is ever modified. Consider adding a mkdir task before the copy operation.
| try: | ||
| if sys.version_info >= (3, 9): | ||
| from importlib.resources import files, as_file | ||
| resource = files('cqlshlib.resources').joinpath('CQL.html') | ||
| with as_file(resource) as path: | ||
| if path.exists(): | ||
| return 'file://' + str(path.resolve()) | ||
| else: | ||
| # Python 3.8 compatibility | ||
| from importlib.resources import path as resource_path | ||
| with resource_path('cqlshlib.resources', 'CQL.html') as path: | ||
| if path.exists(): | ||
| return 'file://' + str(path.resolve()) |
There was a problem hiding this comment.
The context manager pattern here is problematic. The 'as_file' context manager may create temporary files that are cleaned up when exiting the context (which happens before returning). Since the returned URL will be used later when users run HELP commands, the file may no longer exist.
The proper approach is to check if the resource exists before using as_file, and ensure the resource is accessible. For Python 3.9+, you can use 'files()' to get a Traversable and check 'is_file()' on it. If it exists in a package that's already on the filesystem, as_file will return the actual path (no cleanup issue). However, if the package is in a zip file, the extracted temp file will be cleaned up immediately after return.
Consider one of these alternatives:
- Check resource.is_file() before using as_file, and handle the zip case differently
- Extract and cache the resource to a permanent location at startup
- Keep the context manager open for the shell's lifetime by managing it at a higher scope
| with resource_path('cqlshlib.resources', 'CQL.html') as path: | ||
| if path.exists(): | ||
| return 'file://' + str(path.resolve()) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Any error while loading the bundled CQL docs is non-fatal; fall back to other locations. |
1 participant
Fixes macOS compatibility for
cqlsh HELPcommand by bundlingCQL.htmlas a Python package resource. This is meant to be a second attempt of the approach in #2592.On macOS,
/usr/shareis not writable, and/usr/local/shareis the preferred location instead. This PR:/usr/local/sharepath as a fallback for macOS, andChanges
pylib/cqlshlib/resources/__init__.py: New subpackage for bundled resourcespylib/setup.py: Addedpackage_datato include CQL.html/CSSpylib/cqlshlib/cqlshmain.py: Updatedget_docspath()with updated retrieval logic/priority of docs path (ordered by priority):/usr/share/doc/cassandra/)/usr/local/share/doc/cassandra/)importlib.resourcesbuild.xml: Added target to copy CQL docs to pylib during build.gitignore: Ignore generated resource filespylib/cqlshlib/test/test_docspath.py: Unit tests for the new functionalityTesting
ant buildcreatespylib/cqlshlib/resources/CQL.{html,css}bin/cqlsh -e "HELP select”opens appropiate path given different install locations ofCQL.{html,css}