From 35e01b0236ea934520d438e5ed0e8771fdd03cf1 Mon Sep 17 00:00:00 2001 From: Sourabh Kondapaka Date: Thu, 11 Jan 2024 07:21:54 +0000 Subject: [PATCH] matlab-proxy now warns instead of erroring out when Xvfb is not found on Linux Systems. * Makes Xvfb optional on Linux. * Added infrastructure to display warnings on the status information modal. fixes mathworks/jupyter-matlab-proxy#75 --- README.md | 3 +- gui/package.json | 2 +- .../components/Information/Information.css | 20 +-- gui/src/components/Information/index.js | 36 ++++- gui/src/reducers/index.js | 11 ++ gui/src/selectors/index.js | 1 + matlab_proxy/app.py | 1 + matlab_proxy/app_state.py | 27 +++- matlab_proxy/settings.py | 12 +- tests/unit/test_app_state.py | 138 +++++++++++++----- tests/unit/util/test_mw.py | 7 +- 11 files changed, 199 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index e5d9e44b..9da9828d 100644 --- a/README.md +++ b/README.md @@ -47,9 +47,10 @@ The MATLAB Proxy is under active development. For support or to report issues, s $ sudo yum install xorg-x11-server-Xvfb ``` + + *Note: The installation of Xvfb is **optional** (w.e.f. v0.11.0 of matlab-proxy). However, we highly recommend installing it.* * Python versions: **3.8** | **3.9** | **3.10** | **3.11** * [Browser Requirements](https://www.mathworks.com/support/requirements/browser-requirements.html) - * Supported Operating Systems: * Linux® * Windows® Operating System ( starting v0.4.0 of matlab-proxy ) diff --git a/gui/package.json b/gui/package.json index e69898c8..b9709f97 100644 --- a/gui/package.json +++ b/gui/package.json @@ -40,7 +40,7 @@ "last 1 safari version" ] }, - "proxy": "http://127.0.0.1:8000", + "proxy": "http://127.0.0.1:8888", "homepage": "./", "devDependencies": { "@babel/preset-env": "^7.11.0", diff --git a/gui/src/components/Information/Information.css b/gui/src/components/Information/Information.css index 6ca28b4b..e111150d 100644 --- a/gui/src/components/Information/Information.css +++ b/gui/src/components/Information/Information.css @@ -1,6 +1,6 @@ /* Copyright (c) 2020-2022 The MathWorks, Inc. */ -#information .alert:not(.error-container) { +#information .alert:not(.error-container, .warnings-container) { padding: 0; margin-bottom: 0; background: white; @@ -24,37 +24,39 @@ margin-bottom: 0; } -#information .error-msg { +#information .error-msg, .warnings-msg { white-space: pre; font: 10pt monospace; } -#information .error-container.alert { +#information .error-container.alert, .warnings-container.alert { border-radius: 0; margin: 5px 20px; padding: 10px; } -#information .error-container.alert p { +#information .error-container.alert p, .warnings-container.alert p { margin: 0; } -#information .error-container.alert .error-text { +#information .error-container.alert .error-text, +.warnings-container.alert .warnings-text { margin-left: 30px; } -#information .error-logs-container .error-container.alert { +#information .error-logs-container .error-container.alert, +.warnings-container .warnings-container.alert { padding: 0; /* if this is inside an expand-collapse, padding just gets in the way */ } -#information .error-msg { +#information .error-msg, .warnings-msg { padding: 10px; max-height: 200px; overflow: auto; } -#information .error-logs-container .expand_trigger { +#information .error-logs-container .warnings-container .expand_trigger { padding: 10px 5px 5px 40px; font-size: 15px; } @@ -156,4 +158,4 @@ padding-top: 5px; padding-bottom: 0.3em; max-width: 10ch; -} \ No newline at end of file +} diff --git a/gui/src/components/Information/index.js b/gui/src/components/Information/index.js index b04e2ddc..250200e8 100644 --- a/gui/src/components/Information/index.js +++ b/gui/src/components/Information/index.js @@ -7,6 +7,7 @@ import Linkify from 'react-linkify'; import { selectLicensingInfo, selectError, + selectWarnings, selectOverlayHidable, selectInformationDetails, selectAuthEnabled, @@ -22,6 +23,7 @@ function Information({ }) { const licensingInfo = useSelector(selectLicensingInfo); const error = useSelector(selectError); + const warnings = useSelector(selectWarnings); const overlayHidable = useSelector(selectOverlayHidable); const [token, setToken] = useState(''); @@ -33,9 +35,7 @@ function Information({ const tokenInput = useRef(); const [errorLogsExpanded, setErrorLogsExpanded] = useState(false); - const errorLogsExpandedToggle = () => { - setErrorLogsExpanded(!errorLogsExpanded); - }; + const [warningsExpanded, setWarningsExpanded] = useState(true); let info; switch (licensingInfo?.type) { @@ -73,8 +73,8 @@ function Information({ const errorLogsNode = (error && error.logs !== null && error.logs.length > 0) ? (
-

+

setErrorLogsExpanded(!errorLogsExpanded)}> Error logs @@ -89,6 +89,31 @@ function Information({

) : null; + const linkDecorator = (href, text, key) => ( + + {text} + + ); + + + const warningsNode = (warnings && warnings.length > 0) ? ( +
+

setWarningsExpanded(!warningsExpanded)}> + + + Warnings +

+
+ +
{warnings.map((warning, index) => (index + 1).toString() + ")" + warning.trim()).join("\n\n")}
+ +
+
+ ) : null; + const onCloseClick = event => { if (event.target === event.currentTarget) { event.preventDefault(); @@ -197,6 +222,7 @@ function Information({ {errorMessageNode} {errorLogsNode} + {warningsNode}
{children} diff --git a/gui/src/reducers/index.js b/gui/src/reducers/index.js index 9271b4dc..c86bb121 100644 --- a/gui/src/reducers/index.js +++ b/gui/src/reducers/index.js @@ -253,6 +253,16 @@ export function loadUrl(state = null, action) { } } +export function warnings(state = null, action) { + switch (action.type) { + case RECEIVE_SERVER_STATUS: + const warnings = action.status.warnings; + return warnings.length > 0 ? warnings : null; + default: + return state; + } +} + export function error(state = null, action) { switch (action.type) { case SET_AUTH_STATUS: @@ -328,6 +338,7 @@ export default combineReducers({ serverStatus, loadUrl, error, + warnings, envConfig, useMOS, useMRE, diff --git a/gui/src/selectors/index.js b/gui/src/selectors/index.js index 9538190c..c0f48d23 100644 --- a/gui/src/selectors/index.js +++ b/gui/src/selectors/index.js @@ -15,6 +15,7 @@ export const selectLicensingInfo = state => state.serverStatus.licensingInfo; export const selectServerStatusFetchFailCount = state => state.serverStatus.fetchFailCount; export const selectLoadUrl = state => state.loadUrl; export const selectError = state => state.error; +export const selectWarnings = state => state.warnings; export const selectUseMOS = state => state.useMOS === true; export const selectUseMRE = state => state.useMRE === true; export const selectAuthEnabled = state => state.authentication.enabled; diff --git a/matlab_proxy/app.py b/matlab_proxy/app.py index 2ed929d4..94c36b9e 100644 --- a/matlab_proxy/app.py +++ b/matlab_proxy/app.py @@ -113,6 +113,7 @@ async def create_status_response(app, loadUrl=None): "licensing": marshal_licensing_info(state.licensing), "loadUrl": loadUrl, "error": marshal_error(state.error), + "warnings": state.warnings, "wsEnv": state.settings.get("ws_env", ""), } ) diff --git a/matlab_proxy/app_state.py b/matlab_proxy/app_state.py index 2ce9bcd4..711fdd1c 100644 --- a/matlab_proxy/app_state.py +++ b/matlab_proxy/app_state.py @@ -84,6 +84,7 @@ def __init__(self, settings): # Initialize with the error state from the initialization of settings self.error = settings["error"] + self.warnings = settings["warnings"] if self.error is not None: self.logs["matlab"].clear() @@ -262,7 +263,10 @@ def _are_required_processes_ready( xvfb_process = self.processes["xvfb"] if system.is_linux(): - if xvfb_process is None or xvfb_process.returncode is not None: + # If Xvfb is on system PATH, check if it up and running. + if self.settings["is_xvfb_available"] and ( + xvfb_process is None or xvfb_process.returncode is not None + ): logger.debug( "Xvfb has not started" if xvfb_process is None @@ -671,9 +675,22 @@ async def __setup_env_for_matlab(self) -> dict: # DDUX info for MATLAB matlab_env["MW_CONTEXT_TAGS"] = self.settings.get("mw_context_tags") + # Update DISPLAY env variable for MATLAB only if it was supplied by Xvfb. if system.is_linux(): - # Adding DISPLAY key which is only available after starting Xvfb successfully. - matlab_env["DISPLAY"] = self.settings["matlab_display"] + if self.settings.get("matlab_display", None): + matlab_env["DISPLAY"] = self.settings["matlab_display"] + logger.info( + f"Using the display number supplied by Xvfb process:{matlab_env['DISPLAY']} for launching MATLAB" + ) + else: + if "DISPLAY" in matlab_env: + logger.info( + f"Using the existing DISPLAY environment variable with value:{matlab_env['DISPLAY']} for launching MATLAB" + ) + else: + logger.info( + "No DISPLAY environment variable found. Launching MATLAB without it." + ) # The matlab ready file is written into this location(self.mwi_logs_dir) by MATLAB # The mwi_logs_dir is where MATLAB will write any subsequent logs @@ -968,8 +985,8 @@ async def start_matlab(self, restart_matlab=False): self.error = None self.logs["matlab"].clear() - # Start Xvfb process if in a posix system - if system.is_linux(): + # Start Xvfb process on linux if possible + if system.is_linux() and self.settings["is_xvfb_available"]: xvfb = await self.__start_xvfb_process() # xvfb variable would be None if creation of the process failed. diff --git a/matlab_proxy/settings.py b/matlab_proxy/settings.py index 62b6a497..57bf9c34 100644 --- a/matlab_proxy/settings.py +++ b/matlab_proxy/settings.py @@ -199,6 +199,8 @@ def get_dev_settings(config): "mwi_use_existing_license": mwi.validators.validate_use_existing_licensing( os.getenv(mwi_env.get_env_name_mwi_use_existing_license(), "") ), + "warnings": [], + "is_xvfb_available": False, } @@ -243,13 +245,21 @@ def get(config_name=matlab_proxy.get_default_config_name(), dev=False): settings["nlm_conn_str"] = "123@nlm" else: - settings = {"error": None} + settings = {"error": None, "warnings": []} # Initializing server settings separately allows us to return # a minimal set of settings required to launch the server even if # there is an exception thrown when creating the matlab specific settings. settings.update(get_server_settings(config_name)) + settings["is_xvfb_available"] = True if shutil.which("Xvfb") else False + + # Warn user if xvfb is not available on system path. + if system.is_linux() and not settings["is_xvfb_available"]: + warning = " Unable to find Xvfb on the system PATH. Xvfb enables graphical abilities like plots and figures in the MATLAB desktop.\nConsider adding Xvfb to the system PATH and restart matlab-proxy. See https://github.com/mathworks/matlab-proxy#requirements for information." + logger.warning(warning) + settings["warnings"].append(warning) + try: # Update settings with matlab specific values. settings.update(get_matlab_settings()) diff --git a/tests/unit/test_app_state.py b/tests/unit/test_app_state.py index a3116b7b..e82c7760 100644 --- a/tests/unit/test_app_state.py +++ b/tests/unit/test_app_state.py @@ -7,6 +7,7 @@ from typing import Optional import pytest +from matlab_proxy import settings from matlab_proxy.app_state import AppState from matlab_proxy.util.mwi.exceptions import LicensingError, MatlabError @@ -14,14 +15,41 @@ @pytest.fixture -def app_state_fixture(): +def sample_settings_fixture(tmp_path): + """A pytest fixture which returns a dict containing sample settings for the AppState class. + + Args: + tmp_path : Builtin pytest fixture + + Returns: + dict: A dictionary of sample settings + """ + tmp_file = tmp_path / "parent_1" / "parent_2" / "tmp_file.json" + return { + "error": None, + "warnings": [], + "matlab_config_file": tmp_file, + "is_xvfb_available": True, + "mwi_server_url": "dummy", + "mwi_logs_root_dir": Path(settings.get_mwi_config_folder(dev=True)), + "app_port": 12345, + "mwapikey": "asdf", + } + + +@pytest.fixture +def app_state_fixture(sample_settings_fixture): """A pytest fixture which returns an instance of AppState class with no errors. + Args: + sample_settings_fixture (dict): A dictionary of sample settings to be used by + Returns: AppState: An object of the AppState class """ - settings = {"error": None} - app_state = AppState(settings=settings) + app_state = AppState(settings=sample_settings_fixture) + app_state.processes = {"matlab": None, "xvfb": None} + app_state.licensing = {"type": "existing_license"} return app_state @@ -91,6 +119,7 @@ class Mock_xvfb: """An immutable dataclass representing a mocked Xvfb process""" returncode: Optional[int] + pid: Optional[int] @dataclass(frozen=True) @@ -98,6 +127,7 @@ class Mock_matlab: """An immutable dataclass representing a mocked MATLAB process""" returncode: Optional[int] + pid: Optional[int] @pytest.mark.parametrize( @@ -156,7 +186,7 @@ def test_is_licensed(app_state_fixture, licensing, expected): ], ids=["Any error except licensing error", "licensing error"], ) -def test_unset_licensing(err, expected_err): +def test_unset_licensing(err, app_state_fixture, expected_err): """Test to check unset_liecnsing removes licensing from the AppState object Args: @@ -165,34 +195,32 @@ def test_unset_licensing(err, expected_err): expected_err (Exception): Expected exception """ # Arrange - settings = {"error": err} - app_state = AppState(settings=settings) + app_state_fixture.error = err # Act - app_state.unset_licensing() + app_state_fixture.unset_licensing() # Assert - assert app_state.licensing == None - assert type(app_state.error) is type(expected_err) + assert app_state_fixture.licensing == None + assert type(app_state_fixture.error) is type(expected_err) # config file is deleted when licensing info is not set i.e. set to None -def test_persist_licensing_when_licensing_info_is_not_set(tmp_path): +def test_persist_licensing_when_licensing_info_is_not_set(app_state_fixture): """Test to check if data is not persisted to a file if licensing info is not present Args: tmp_path (Path): Built in pytest fixture """ # Arrange - tmp_file = tmp_path / "tmp_file.json" - settings = {"matlab_config_file": tmp_file, "error": None} - app_state = AppState(settings=settings) + # Nothing to arrange + app_state_fixture.licensing = None # Act - app_state.persist_config_data() + app_state_fixture.persist_config_data() # Assert - assert os.path.exists(tmp_file) is False + assert os.path.exists(app_state_fixture.settings["matlab_config_file"]) is False @pytest.mark.parametrize( @@ -221,7 +249,12 @@ def test_persist_config_data(licensing_data: dict, tmp_path): """ # Arrange tmp_file = tmp_path / "parent_1" / "parent_2" / "tmp_file.json" - settings = {"matlab_config_file": tmp_file, "error": None, "matlab_version": None} + settings = { + "matlab_config_file": tmp_file, + "error": None, + "matlab_version": None, + "warnings": [], + } app_state = AppState(settings=settings) app_state.licensing = licensing_data @@ -238,25 +271,31 @@ def test_persist_config_data(licensing_data: dict, tmp_path): validate_required_processes_test_data = [ (None, None, "linux", False), # xvfb is None == True - (None, Mock_xvfb(None), "linux", False), # matlab is None == True + (None, Mock_xvfb(None, 1), "linux", False), # matlab is None == True ( - Mock_matlab(None), - Mock_xvfb(None), + Mock_matlab(None, 1), + Mock_xvfb(None, 1), "linux", True, ), # All branches are skipped and nothing returned ( - Mock_matlab(None), - Mock_xvfb(123), + Mock_matlab(None, 1), + Mock_xvfb(123, 2), "linux", False, ), # xvfb.returncode is not None == True ( - Mock_matlab(123), - Mock_xvfb(None), + Mock_matlab(123, 1), + Mock_xvfb(None, 2), "linux", False, ), # matlab.returncode is not None == True + ( + Mock_matlab(None, 1), + None, + "linux", + True, + ), # Xvfb not found on path ] @@ -269,6 +308,7 @@ def test_persist_config_data(licensing_data: dict, tmp_path): "All_required_processes_running", "All_processes_running_with_xvfb_returning_non_zero_code", "All_processes_running_with_matlab_returning_non_zero_code", + "xvfb_is_optional_matlab_starts_without_it", ], ) def test_are_required_processes_ready( @@ -284,10 +324,12 @@ def test_are_required_processes_ready( expected (bool): Expected return value based on process return code """ # Arrange - # Nothing to arrange + app_state_fixture.processes = {"matlab": matlab, "xvfb": xvfb} + if not xvfb: + app_state_fixture.settings["is_xvfb_available"] = False # Act - actual = app_state_fixture._are_required_processes_ready(matlab, xvfb) + actual = app_state_fixture._are_required_processes_ready() # Assert assert actual == expected @@ -306,7 +348,7 @@ def test_are_required_processes_ready( ids=["connector_up", "connector_down", "connector_up_ready_file_not_present"], ) async def test_get_matlab_status_based_on_connector_status( - mocker, connector_status, ready_file_present, matlab_status + mocker, app_state_fixture, connector_status, ready_file_present, matlab_status ): """Test to check matlab status based on connector status @@ -322,16 +364,11 @@ async def test_get_matlab_status_based_on_connector_status( return_value=connector_status, ) mocker.patch.object(Path, "exists", return_value=ready_file_present) - settings = { - "error": None, - "mwi_server_url": "dummy", - "mwi_is_token_auth_enabled": False, - } - app_state = AppState(settings=settings) - app_state.matlab_session_files["matlab_ready_file"] = Path("dummy") + app_state_fixture.settings["mwi_is_token_auth_enabled"] = False + app_state_fixture.matlab_session_files["matlab_ready_file"] = Path("dummy") # Act - actual_matlab_status = await app_state._get_matlab_connector_status() + actual_matlab_status = await app_state_fixture._get_matlab_connector_status() # Assert assert actual_matlab_status == matlab_status @@ -510,3 +547,36 @@ async def test_requests_sent_by_matlab_proxy_have_headers( "headers" ] assert sample_token_headers_fixture == connector_status_request_headers + + +async def test_start_matlab_without_xvfb(app_state_fixture, mocker): + """Test to check if Matlab process starts without throwing errors when Xvfb is not present + + Args: + app_state_fixture (AppState): Object of AppState class with defaults set + mocker : Built-in pytest fixture + """ + # Arrange + app_state_fixture.settings["is_xvfb_available"] = False + mock_matlab = Mock_matlab(None, 1) + + # Starting asyncio tasks related to matlab is not required here as only Xvfb check is required. + mocker.patch.object( + AppState, "_AppState__start_matlab_process", return_value=mock_matlab + ) + mocker.patch.object( + AppState, "_AppState__matlab_stderr_reader_posix", return_value=None + ) + mocker.patch.object( + AppState, "_AppState__track_embedded_connector_state", return_value=None + ) + mocker.patch.object(AppState, "_AppState__update_matlab_port", return_value=None) + + # Act + await app_state_fixture.start_matlab() + + # Assert + # Check if Xvfb has not started + assert app_state_fixture.processes["xvfb"] is None + # Check if Matlab started + assert app_state_fixture.processes["matlab"] is mock_matlab diff --git a/tests/unit/util/test_mw.py b/tests/unit/util/test_mw.py index 18b2dcad..95708037 100644 --- a/tests/unit/util/test_mw.py +++ b/tests/unit/util/test_mw.py @@ -525,10 +525,11 @@ async def test_create_xvfb_process(loop): Creates 2 xvfb processes with '-displayfd' flag and checks if the processes are running on unique display ports """ - - # Get command to launch xvfb with -displayfd flag. settings_1 = settings.get(dev=True) - settings_2 = settings.get(dev=True) + + # Return if Xvfb is not available + if not settings_1["is_xvfb_available"]: + return xvfb_cmd_1, pipe_1 = settings.create_xvfb_cmd() xvfb_cmd_2, pipe_2 = settings.create_xvfb_cmd()