From d81a6fe4bf93519254f4ef8f7a1c89c8404ebb97 Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Tue, 1 Apr 2025 14:24:57 -0700 Subject: [PATCH 1/8] Get asset information from Github releases page request, including asset size. This can reduce url requests when checking for asset availability. This also obviates writing asset size to file. Verifying download now checks asset size against size info in Github release. Download directory is cleared immediately prior to download. --- .../plugins/updatemanager/widgets/update.py | 70 +++--- spyder/plugins/updatemanager/workers.py | 224 ++++++++++-------- 2 files changed, 156 insertions(+), 138 deletions(-) diff --git a/spyder/plugins/updatemanager/widgets/update.py b/spyder/plugins/updatemanager/widgets/update.py index 6667c62c806..89842149aee 100644 --- a/spyder/plugins/updatemanager/widgets/update.py +++ b/spyder/plugins/updatemanager/widgets/update.py @@ -26,7 +26,6 @@ from spyder.api.translations import _ from spyder.config.base import is_conda_based_app from spyder.plugins.updatemanager.workers import ( - get_asset_info, WorkerUpdate, WorkerDownloadInstaller ) @@ -133,23 +132,22 @@ def __init__(self, parent): self.update_thread = None self.update_worker = None self.update_timer = None - self.latest_release = None + self.asset_info = None self.cancelled = False self.download_thread = None self.download_worker = None self.progress_dialog = None self.installer_path = None - self.installer_size_path = None - - # Type of Spyder update. It can be "major", "minor" or "micro" - self.update_type = None # ---- General def set_status(self, status=NO_STATUS): """Set the update manager status.""" - self.sig_set_status.emit(status, str(self.latest_release)) + version = None + if self.asset_info is not None: + version = self.asset_info["version"] + self.sig_set_status.emit(status, str(version)) def cleanup_threads(self): """Clean up QThreads""" @@ -226,7 +224,7 @@ def start_check_update(self, startup=False): def _process_check_update(self): """Process the results of check update.""" # Get results from worker - update_available = self.update_worker.update_available + update_available = self.update_worker.asset_info is not None error_msg = self.update_worker.error checkbox = self.update_worker.checkbox @@ -250,30 +248,22 @@ def _process_check_update(self): else: info_messagebox(self, _("Spyder is up to date."), checkbox=True) - def _set_installer_path(self): - """Set the temp file path for the downloaded installer.""" - asset_info = get_asset_info(self.latest_release) - self.update_type = asset_info['update_type'] - - dirname = osp.join(get_temp_dir(), 'updates', str(self.latest_release)) - self.installer_path = osp.join(dirname, asset_info['filename']) - self.installer_size_path = osp.join(dirname, "size") + # ---- Download Update - logger.info(f"Update type: {self.update_type}") + def _set_installer_path(self): + dirname = osp.join( + get_temp_dir(), 'updates', str(self.asset_info["version"]) + ) + self.installer_path = osp.join(dirname, self.asset_info['filename']) - # ---- Download Update + logger.info(f"Update type: {self.asset_info['update_type']}") def _verify_installer_path(self): - if ( - osp.exists(self.installer_path) - and osp.exists(self.installer_size_path) - ): - with open(self.installer_size_path, "r") as f: - size = int(f.read().strip()) - - update_downloaded = size == osp.getsize(self.installer_path) - else: - update_downloaded = False + update_downloaded = False + if osp.exists(self.installer_path): + update_downloaded = ( + self.asset_info["size"] == osp.getsize(self.installer_path) + ) logger.debug(f"Update already downloaded: {update_downloaded}") @@ -288,8 +278,9 @@ def start_update(self): If the installer is already downloaded, proceed to confirm install. """ - self.latest_release = self.update_worker.latest_release + self.asset_info = self.update_worker.asset_info self._set_installer_path() + version = self.asset_info["version"] if self._verify_installer_path(): self.set_status(DOWNLOAD_FINISHED) @@ -306,21 +297,19 @@ def start_update(self): ).format(URL_I + "#standalone-installers") box = confirm_messagebox( - self, msg, _('Spyder update'), - version=self.latest_release, checkbox=True + self, msg, _('Spyder update'), version=version, checkbox=True ) if box.result() == QMessageBox.Yes: self._start_download() else: manual_update_messagebox( - self, self.latest_release, self.update_worker.channel + self, version, self.update_worker.channel ) else: msg = _("Would you like to automatically download " "and install it?") box = confirm_messagebox( - self, msg, _('Spyder update'), - version=self.latest_release, checkbox=True + self, msg, _('Spyder update'), version=version, checkbox=True ) if box.result() == QMessageBox.Yes: self._start_download() @@ -334,7 +323,7 @@ def _start_download(self): self.progress_dialog = None self.download_worker = WorkerDownloadInstaller( - self.latest_release, self.installer_path, self.installer_size_path + self.asset_info, self.installer_path ) self.sig_disable_actions.emit(True) @@ -343,7 +332,10 @@ def _start_download(self): # Only show progress bar for installers if not self.installer_path.endswith('zip'): self.progress_dialog = ProgressDialog( - self, _("Downloading Spyder {} ...").format(self.latest_release) + self, + _("Downloading Spyder {} ...").format( + self.asset_info["version"] + ) ) self.progress_dialog.cancel.clicked.connect(self._cancel_download) @@ -422,7 +414,7 @@ def _confirm_install(self): self, msg, _('Spyder install'), - version=self.latest_release, + version=self.asset_info["version"], on_close=True ) if box.result() == QMessageBox.Yes: @@ -444,11 +436,11 @@ def start_install(self): # Sub command sub_cmd = [tmpscript_path, '-i', self.installer_path] - if self.update_type != 'major': + if self.asset_info["update_type"] != 'major': # Update with conda sub_cmd.extend(['-c', find_conda(), '-p', sys.prefix]) - if self.update_type == 'minor': + if self.asset_info["update_type"] == 'minor': # Rebuild runtime environment sub_cmd.append('-r') diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index 01e4f5b0d5d..cea810df369 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -58,6 +58,7 @@ "

The error was:

{error}" ) + def _rate_limits(page): """Log rate limits for GitHub.com""" if page.headers.get('Server') != 'GitHub.com': @@ -86,6 +87,9 @@ class UpdateType: class AssetInfo(TypedDict): """Schema for asset information.""" + # Version + version: Version + # Filename with extension of the release asset to download. filename: str @@ -95,51 +99,85 @@ class AssetInfo(TypedDict): # Download URL for the asset. url: str + # File size + size: int -def get_asset_info(release: str | Version) -> AssetInfo: + +def get_github_releases() -> dict[Version, dict]: """ - Get the name, update type, and download URL for the asset of the given - release. + Get Github release information + + Returns + ------- + releases : dict[packaging.version.Version, dict] + Dictionary of release information. + """ + url = ( + "https://api.github.com/repos/spyder-ide/spyder/releases" + "?per_page=20&page=1" + ) + headers = {} + token = os.getenv('GITHUB_TOKEN') + if running_in_ci() and token: + headers.update(Authorization=f"Bearer {token}") + + logger.info(f"Getting release info from {url}") + page = requests.get(url, headers=headers) + _rate_limits(page) + page.raise_for_status() + data = page.json() + + return {parse(item['tag_name']): item for item in data} + + +def get_asset_info(release_info: dict) -> None | AssetInfo: + """ + Get the version, name, update type, download URL, and size for the asset + of the given release. Parameters ---------- - release: str | packaging.version.Version - Release version + release_info: dict + Release information on Github Returns ------- - asset_info: AssetInfo + asset_info: None | AssetInfo Information about the asset. """ - if isinstance(release, str): - release = parse(release) + release = parse(release_info["tag_name"]) - if CURRENT_VERSION.major < release.major: + if CURRENT_VERSION.major < release.major or not is_conda_based_app(): update_type = UpdateType.Major elif CURRENT_VERSION.minor < release.minor: update_type = UpdateType.Minor else: update_type = UpdateType.Micro - mach = platform.machine().lower().replace("amd64", "x86_64") - if update_type == UpdateType.Major or not is_conda_based_app(): + ext = platform.machine().lower().replace("amd64", "x86_64") if os.name == 'nt': - plat, ext = 'Windows', 'exe' + ext += '.exe' if sys.platform == 'darwin': - plat, ext = 'macOS', 'pkg' + ext += '.pkg' if sys.platform.startswith('linux'): - plat, ext = 'Linux', 'sh' - name = f'Spyder-{plat}-{mach}.{ext}' + ext += '.sh' else: - name = 'spyder-conda-lock.zip' - - url = ( - 'https://github.com/spyder-ide/spyder/releases/download/' - f'v{release}/{name}' - ) + ext = '.zip' + + asset_info = None + for asset in release_info["assets"]: + if asset["name"].endswith(ext): + asset_info = AssetInfo( + version=release, + filename=asset["name"], + update_type=update_type, + url=asset["browser_download_url"], + size=asset["size"] + ) + break - return AssetInfo(filename=name, update_type=update_type, url=url) + return asset_info class UpdateDownloadCancelledException(Exception): @@ -193,25 +231,31 @@ class WorkerUpdate(BaseWorker): def __init__(self, stable_only): super().__init__() self.stable_only = stable_only - self.latest_release = None - self.update_available = False + self.asset_info = None self.error = None self.checkbox = False self.channel = None - def _check_update_available( - self, - releases: list[Version], - github: bool = True - ): - """Checks if there is an update available from releases.""" + def _check_asset_available(self, release: None | Version): + """Checks if there is an asset available for release.""" + + # Get asset info from Github + releases = get_github_releases() + + if release is not None: + # Only keep Github releases that are also available at channel url + releases = {k: v for k, v in releases.items() if k <= release} + if self.stable_only: # Only use stable releases - releases = [r for r in releases if not r.is_prerelease] - logger.debug(f"Available versions: {releases}") + releases = { + k: v for k, v in releases.items() if not k.is_prerelease + } + logger.debug(f"Available versions: {sorted(releases)}") latest_release = max(releases) if releases else CURRENT_VERSION update_available = CURRENT_VERSION < latest_release + asset_info = None logger.debug(f"Latest release: {latest_release}") logger.debug(f"Update available: {update_available}") @@ -220,77 +264,56 @@ def _check_update_available( # If the asset is not available, then check the next latest # release, and so on until either a new asset is available or there # is no update available. - if github: - asset_available = False - while update_available and not asset_available: - asset_info = get_asset_info(latest_release) - page = requests.head(asset_info['url']) - if page.status_code == 302: - # The asset is found - logger.debug(f"Asset available for url: {page.url}") - asset_available = True - else: - # The asset is not available - logger.debug( - "Asset not available: " - f"{page.status_code} Client Error: {page.reason} " - f"for url: {page.url}" - ) - asset_available = False - releases.remove(latest_release) - - latest_release = max(releases) if releases else CURRENT_VERSION - update_available = CURRENT_VERSION < latest_release - - logger.debug(f"Latest release: {latest_release}") - logger.debug(f"Update available: {update_available}") - - self.latest_release = latest_release - self.update_available = update_available + asset_available = False + while update_available and not asset_available: + asset_info = get_asset_info(releases.get(latest_release)) + + if asset_info is not None: + # The asset is available + logger.debug(f"Asset available for url: {asset_info['url']}") + asset_available = True + else: + # The asset is not available + logger.debug(f"Asset not available: {latest_release}") + asset_available = False + releases.pop(latest_release) + + latest_release = max(releases) if releases else CURRENT_VERSION + update_available = CURRENT_VERSION < latest_release + + logger.debug(f"Latest release: {latest_release}") + logger.debug(f"Update available: {update_available}") + + self.asset_info = asset_info def start(self): """Main method of the worker.""" self.error = None - self.latest_release = None - self.update_available = False + self.asset_info = None error_msg = None - url = 'https://api.github.com/repos/spyder-ide/spyder/releases' + url = None if not is_conda_based_app(): self.channel = "pypi" # Default channel if not conda if is_conda_env(sys.prefix): self.channel, channel_url = get_spyder_conda_channel() - - # If Spyder is installed from defaults channel (pkgs/main), then - # use that channel to get updates. The defaults channel can be far - # behind our latest release. - if self.channel == "pkgs/main": url = channel_url + '/channeldata.json' - github = "api.github.com" in url - headers = {} - token = os.getenv('GITHUB_TOKEN') - if running_in_ci() and token: - headers.update(Authorization=f"Bearer {token}") - - logger.info(f"Checking for updates from {url}") try: - page = requests.get(url, headers=headers) - _rate_limits(page) - page.raise_for_status() - - data = page.json() - if github: - # Github url - releases = [parse(item['tag_name']) for item in data] - else: - # Conda pkgs/main url + release = None + if url is not None: + logger.info(f"Getting release from {url}") + page = requests.get(url) + page.raise_for_status() + data = page.json() + + # Conda pkgs/main or conda-forge url + # What about PyPi? spyder_data = data['packages'].get('spyder') if spyder_data: - releases = [parse(spyder_data["version"])] - releases.sort() + release = parse(spyder_data["version"]) - self._check_update_available(releases, github) + self._check_asset_available(release) except SSLError as err: error_msg = SSL_ERROR_MSG @@ -299,7 +322,8 @@ def start(self): error_msg = CONNECT_ERROR_MSG logger.warning(err, exc_info=err) except HTTPError as err: - error_msg = HTTP_ERROR_MSG.format(status_code=page.status_code) + status_code = err.response.status_code + error_msg = HTTP_ERROR_MSG.format(status_code=status_code) logger.warning(err, exc_info=err) except OSError as err: error_msg = OS_ERROR_MSG.format(error=err) @@ -345,11 +369,10 @@ class WorkerDownloadInstaller(BaseWorker): Total size of the file expected to be downloaded. """ - def __init__(self, latest_release, installer_path, installer_size_path): + def __init__(self, asset_info, installer_path): super().__init__() - self.latest_release = latest_release + self.asset_info = asset_info self.installer_path = installer_path - self.installer_size_path = installer_size_path self.error = None self.cancelled = False self.paused = False @@ -366,18 +389,23 @@ def _progress_reporter(self, progress, total_size): def _download_installer(self): """Donwload Spyder installer.""" - asset_info = get_asset_info(self.latest_release) - url = asset_info['url'] + url = self.asset_info["url"] logger.info(f"Downloading {url} to {self.installer_path}") + self._clean_installer_dir() dirname = osp.dirname(self.installer_path) os.makedirs(dirname, exist_ok=True) with requests.get(url, stream=True) as r: r.raise_for_status() - size = -1 - if "content-length" in r.headers: - size = int(r.headers["content-length"]) + + size = int(r.headers["content-length"]) + exp_size = self.asset_info["size"] + if exp_size != size: + raise UpdateDownloadIncompleteError( + f"Download error: size = {size} bytes; " + f"expected {exp_size} bytes" + ) self._progress_reporter(0, size) with open(self.installer_path, 'wb') as f: @@ -388,10 +416,8 @@ def _download_installer(self): f.write(chunk) self._progress_reporter(size_read, size) - if size_read == size: + if size_read == exp_size: logger.info('Download successfully completed.') - with open(self.installer_size_path, "w") as f: - f.write(str(size)) if self.installer_path.endswith('.zip'): with ZipFile(self.installer_path, 'r') as f: From 3e93595de232c9f38e0bd5beeb8bddf554c00c0a Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Tue, 1 Apr 2025 12:02:09 -0700 Subject: [PATCH 2/8] Update unit tests --- .../tests/test_update_manager.py | 70 +++++++++---------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index 3a8da7c6c3f..89033b07d43 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -23,14 +23,11 @@ @pytest.fixture(autouse=True) def capture_logging(caplog): + # Capture >=DEBUG logging messages for spyder.plugins.updatemanager. + # Messages will be reported at the end of the pytest run for failed tests. caplog.set_level(10, "spyder.plugins.updatemanager") -@pytest.fixture -def worker(): - return WorkerUpdate(None) - - # ---- Test WorkerUpdate @pytest.mark.parametrize("version", ["1.0.0", "1000.0.0"]) @@ -57,13 +54,9 @@ def test_updates(qtbot, mocker, caplog, version, channel): workers, "get_spyder_conda_channel", return_value=channel ) - with caplog.at_level(logging.DEBUG, logger='spyder.plugins.updatemanager'): - # Capture >=DEBUG logging messages for spyder.plugins.updatemanager - # while checking for updates. Messages will be reported at the end - # of the pytest run, and only if this test fails. - um = UpdateManagerWidget(None) - um.start_check_update() - qtbot.waitUntil(um.update_thread.isFinished) + um = UpdateManagerWidget(None) + um.start_check_update() + qtbot.waitUntil(um.update_thread.isFinished) if um.update_worker.error: # Possible 403 error - rate limit error, was encountered while doing @@ -73,17 +66,14 @@ def test_updates(qtbot, mocker, caplog, version, channel): assert um.update_worker.error == HTTP_ERROR_MSG.format(status_code="403") return - assert not um.update_worker.error - - update_available = um.update_worker.update_available if version.split('.')[0] == '1': - assert update_available + assert um.update_worker.asset_info is not None else: - assert not update_available + assert um.update_worker.asset_info is None -@pytest.mark.parametrize("release", ["6.0.0", "6.0.0b3"]) @pytest.mark.parametrize("version", ["4.0.0a1", "4.0.0"]) +@pytest.mark.parametrize("release", ["6.0.0", "6.0.0b3"]) @pytest.mark.parametrize("stable_only", [True, False]) def test_update_non_stable(qtbot, mocker, version, release, stable_only): """Test we offer unstable updates.""" @@ -91,44 +81,51 @@ def test_update_non_stable(qtbot, mocker, version, release, stable_only): release = parse(release) worker = WorkerUpdate(stable_only) - worker._check_update_available([release]) + worker._check_asset_available(release) if release.is_prerelease and stable_only: - assert not worker.update_available + assert worker.asset_info is None else: - assert worker.update_available + assert worker.asset_info is not None @pytest.mark.parametrize("version", ["4.0.0", "6.0.0"]) -def test_update_no_asset(qtbot, mocker, version): +@pytest.mark.parametrize("release", [None, "6.0.1", "6.100.0"]) +def test_update_no_asset(qtbot, mocker, version, release): """Test update availability when asset is not available""" mocker.patch.object(workers, "CURRENT_VERSION", new=parse(version)) - releases = [parse("6.0.1"), parse("6.100.0")] + release = parse(release) if release else None worker = WorkerUpdate(True) - worker._check_update_available(releases) + worker._check_asset_available(release) # For both values of version, there should be an update available # However, the available version should be 6.0.1, since there is # no asset for 6.100.0 - assert worker.update_available - assert worker.latest_release == releases[0] + assert worker.asset_info is not None + assert worker.asset_info["version"] >= parse("6.0.1") @pytest.mark.parametrize( - "release,update_type", + "app,version,release,update_type", [ - ("6.0.1", UpdateType.Micro), - ("6.1.0", UpdateType.Minor), - ("7.0.0", UpdateType.Major) + (True, "6.0.0", "6.0.1", UpdateType.Micro), + (True, "6.0.0", "6.1.0a1", UpdateType.Minor), + (True, "5.0.0", "6.0.0", UpdateType.Major), + (False, "6.0.0", "6.0.1", UpdateType.Major), + (False, "6.0.0", "6.1.0a1", UpdateType.Major), + (False, "5.0.0", "6.0.0", UpdateType.Major) ] ) -@pytest.mark.parametrize("app", [True, False]) -def test_get_asset_info(qtbot, mocker, release, update_type, app): - mocker.patch.object(workers, "CURRENT_VERSION", new=parse("6.0.0")) +def test_get_asset_info(qtbot, mocker, app, version, release, update_type): + mocker.patch.object(workers, "CURRENT_VERSION", new=parse(version)) mocker.patch.object(workers, "is_conda_based_app", return_value=app) - info = get_asset_info(release) + worker = WorkerUpdate(False) + # import pdb; pdb.set_trace() + worker._check_asset_available(parse(release)) + info = worker.asset_info + assert info['update_type'] == update_type if update_type == "major" or not app: @@ -149,8 +146,11 @@ def test_download(qtbot, mocker): Uses UpdateManagerWidget in order to also test QThread. """ + releases = workers.get_github_releases() + release_info = releases[parse("6.0.0a2")] + um = UpdateManagerWidget(None) - um.latest_release = "6.0.0a2" + um.asset_info = get_asset_info(release_info) um._set_installer_path() # Do not execute _start_install after download completes. From d8a225243c11925a04e3058596b4e8b33704c913 Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Tue, 1 Apr 2025 12:23:29 -0700 Subject: [PATCH 3/8] Cache results from get_github_releases, but only in unit tests. This eliminates the need to consider rate limits on Github url requests. --- .../tests/test_update_manager.py | 13 ++++--------- spyder/plugins/updatemanager/workers.py | 19 ------------------- 2 files changed, 4 insertions(+), 28 deletions(-) diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index 89033b07d43..5bcdfa4f87c 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -5,6 +5,7 @@ # (see spyder/__init__.py for details) import os +from functools import lru_cache import logging from packaging.version import parse @@ -13,13 +14,15 @@ from spyder.config.base import running_in_ci from spyder.plugins.updatemanager import workers from spyder.plugins.updatemanager.workers import ( - UpdateType, get_asset_info, WorkerUpdate, HTTP_ERROR_MSG + UpdateType, get_asset_info, WorkerUpdate ) from spyder.plugins.updatemanager.widgets import update from spyder.plugins.updatemanager.widgets.update import UpdateManagerWidget logging.basicConfig() +workers.get_github_releases = lru_cache(workers.get_github_releases) + @pytest.fixture(autouse=True) def capture_logging(caplog): @@ -58,14 +61,6 @@ def test_updates(qtbot, mocker, caplog, version, channel): um.start_check_update() qtbot.waitUntil(um.update_thread.isFinished) - if um.update_worker.error: - # Possible 403 error - rate limit error, was encountered while doing - # the tests - # Check error message corresponds to the status code and exit early to - # prevent failing the test - assert um.update_worker.error == HTTP_ERROR_MSG.format(status_code="403") - return - if version.split('.')[0] == '1': assert um.update_worker.asset_info is not None else: diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index cea810df369..8abf1de35cc 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -6,7 +6,6 @@ # Standard library imports from __future__ import annotations # noqa; required for typing in Python 3.8 -from datetime import datetime as dt import logging import os import os.path as osp @@ -59,23 +58,6 @@ ) -def _rate_limits(page): - """Log rate limits for GitHub.com""" - if page.headers.get('Server') != 'GitHub.com': - return - - xrlr = dt.utcfromtimestamp(int(page.headers['X-RateLimit-Reset'])) - msg_items = [ - "Rate Limits:", - f"Resource: {page.headers['X-RateLimit-Resource']}", - f"Reset: {xrlr}", - f"Limit: {page.headers['X-RateLimit-Limit']:>5s}", - f"Used: {page.headers['X-RateLimit-Used']:>5s}", - f"Remaining: {page.headers['X-RateLimit-Remaining']:>5s}", - ] - logger.debug("\n\t".join(msg_items)) - - class UpdateType: """Enum with the different update types.""" @@ -123,7 +105,6 @@ def get_github_releases() -> dict[Version, dict]: logger.info(f"Getting release info from {url}") page = requests.get(url, headers=headers) - _rate_limits(page) page.raise_for_status() data = page.json() From 7a524ed062d2535bb764f2a40eb203c696bb82c9 Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Tue, 1 Apr 2025 14:24:57 -0700 Subject: [PATCH 4/8] Use sha256 checksum to validate download instead of file size. Use recommended Github request header. Cache get_asset_checksum, but only in unit tests. Create single checksum file and include digest for zip and pkg assets. --- .github/workflows/installers-conda.yml | 25 ++-- .../tests/test_update_manager.py | 3 +- .../plugins/updatemanager/widgets/update.py | 9 +- spyder/plugins/updatemanager/workers.py | 132 ++++++++++++------ 4 files changed, 112 insertions(+), 57 deletions(-) diff --git a/.github/workflows/installers-conda.yml b/.github/workflows/installers-conda.yml index b24ab425b61..60dfadd995f 100644 --- a/.github/workflows/installers-conda.yml +++ b/.github/workflows/installers-conda.yml @@ -355,15 +355,10 @@ jobs: EXIT /B %ERRORLEVEL% - - name: Notarize or Compute Checksum - if: env.NOTARIZE == 'true' + - name: Notarize + if: runner.os == 'macOS' && env.NOTARIZE == 'true' run: | - if [[ $RUNNER_OS == "macOS" ]]; then - ./notarize.sh -p $APPLICATION_PWD $PKG_PATH - else - cd $DISTDIR - echo $(sha256sum $PKG_NAME) > "${ARTIFACT_NAME}-sha256sum.txt" - fi + ./notarize.sh -p $APPLICATION_PWD $PKG_PATH - name: Upload Artifact uses: actions/upload-artifact@v4 @@ -390,11 +385,21 @@ jobs: - name: Zip Lock Files run: zip -mT spyder-conda-lock *.lock - - name: Upload Artifact + - name: Create Checksums + run: | + sha256sum *.zip *.pkg *.sh *.exe > Spyder-checksums.txt + + - name: Upload Lock Files uses: actions/upload-artifact@v4 with: path: spyder-conda-lock.zip - name: spyder-conda-lock + name: spyder-conda-lock-artifact + + - name: Upload Checksums + uses: actions/upload-artifact@v4 + with: + path: Spyder-checksums.txt + name: Spyder-checksums - name: Get Release if: env.IS_RELEASE == 'true' diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index 5bcdfa4f87c..1db2662cbf2 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -22,6 +22,7 @@ logging.basicConfig() workers.get_github_releases = lru_cache(workers.get_github_releases) +workers.get_asset_checksum = lru_cache(workers.get_asset_checksum) @pytest.fixture(autouse=True) @@ -59,7 +60,7 @@ def test_updates(qtbot, mocker, caplog, version, channel): um = UpdateManagerWidget(None) um.start_check_update() - qtbot.waitUntil(um.update_thread.isFinished) + qtbot.waitUntil(um.update_thread.isFinished, timeout=10000) if version.split('.')[0] == '1': assert um.update_worker.asset_info is not None diff --git a/spyder/plugins/updatemanager/widgets/update.py b/spyder/plugins/updatemanager/widgets/update.py index 89842149aee..ab7aa012e84 100644 --- a/spyder/plugins/updatemanager/widgets/update.py +++ b/spyder/plugins/updatemanager/widgets/update.py @@ -26,6 +26,7 @@ from spyder.api.translations import _ from spyder.config.base import is_conda_based_app from spyder.plugins.updatemanager.workers import ( + validate_download, WorkerUpdate, WorkerDownloadInstaller ) @@ -258,11 +259,11 @@ def _set_installer_path(self): logger.info(f"Update type: {self.asset_info['update_type']}") - def _verify_installer_path(self): + def _validate_download(self): update_downloaded = False if osp.exists(self.installer_path): - update_downloaded = ( - self.asset_info["size"] == osp.getsize(self.installer_path) + update_downloaded = validate_download( + self.installer_path, self.asset_info["checksum"] ) logger.debug(f"Update already downloaded: {update_downloaded}") @@ -282,7 +283,7 @@ def start_update(self): self._set_installer_path() version = self.asset_info["version"] - if self._verify_installer_path(): + if self._validate_download(): self.set_status(DOWNLOAD_FINISHED) self._confirm_install() elif not is_conda_based_app(): diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index 8abf1de35cc..4206ab01583 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -6,6 +6,7 @@ # Standard library imports from __future__ import annotations # noqa; required for typing in Python 3.8 +from hashlib import sha256 import logging import os import os.path as osp @@ -57,6 +58,11 @@ "

The error was:

{error}" ) +GH_HEADERS = {"Accept": "application/vnd.github+json"} +_token = os.getenv('GITHUB_TOKEN') +if running_in_ci() and _token: + GH_HEADERS.update(Authorization=f"Bearer {_token}") + class UpdateType: """Enum with the different update types.""" @@ -81,8 +87,11 @@ class AssetInfo(TypedDict): # Download URL for the asset. url: str - # File size - size: int + # Checksum url + checksum_url: str + + # File sha256 checksum + checksum: str def get_github_releases() -> dict[Version, dict]: @@ -98,19 +107,44 @@ def get_github_releases() -> dict[Version, dict]: "https://api.github.com/repos/spyder-ide/spyder/releases" "?per_page=20&page=1" ) - headers = {} - token = os.getenv('GITHUB_TOKEN') - if running_in_ci() and token: - headers.update(Authorization=f"Bearer {token}") - logger.info(f"Getting release info from {url}") - page = requests.get(url, headers=headers) + page = requests.get(url, headers=GH_HEADERS) page.raise_for_status() data = page.json() return {parse(item['tag_name']): item for item in data} +def get_asset_checksum(url: str, name: str) -> str | None: + """ + Get the checksum for the provided asset. + + Parameters + ---------- + url : str + Url to the checksum asset + name : str + Name of the asset for which to obtain the checksum + + Returns + ------- + checksum : str | None + Checksum for the provided asset. None is returned if the checksum is + not available for the provided asset name. + """ + logger.info(f"Getting checksum from {url}") + page = requests.get(url, headers=GH_HEADERS) + page.raise_for_status() + + digest = page.text.strip().split("\n") + data = {k: v for v, k in [item.split() for item in digest]} + checksum = data.get(name, None) + + logger.info(f"Checksum for {name}: {checksum}") + + return checksum + + def get_asset_info(release_info: dict) -> None | AssetInfo: """ Get the version, name, update type, download URL, and size for the asset @@ -119,7 +153,7 @@ def get_asset_info(release_info: dict) -> None | AssetInfo: Parameters ---------- release_info: dict - Release information on Github + Release information from Github for a single release Returns ------- @@ -146,27 +180,45 @@ def get_asset_info(release_info: dict) -> None | AssetInfo: else: ext = '.zip' - asset_info = None + asset_info = AssetInfo(version=release, update_type=update_type) for asset in release_info["assets"]: if asset["name"].endswith(ext): - asset_info = AssetInfo( - version=release, + asset_info.update( filename=asset["name"], - update_type=update_type, - url=asset["browser_download_url"], - size=asset["size"] + url=asset["browser_download_url"] + ) + if asset["name"] == "Spyder-checksums.txt": + asset_info.update( + checksum_url=asset["browser_download_url"] ) + if asset_info.get("url") and asset_info.get("checksum_url"): break + if asset_info.get("url") is None or asset_info.get("checksum_url") is None: + # Both assets are required + asset_info = None + return asset_info +def validate_download(file, checksum): + with open(file, "rb") as f: + _checksum = sha256() + while chunk := f.read(8192): + _checksum.update(chunk) + + valid = checksum == _checksum.hexdigest() + logger.debug(f"Valid {file}: {valid}") + + return valid + + class UpdateDownloadCancelledException(Exception): """Download for installer to update was cancelled.""" pass -class UpdateDownloadIncompleteError(Exception): +class UpdateDownloadError(Exception): """Error occured while downloading file""" pass @@ -232,7 +284,7 @@ def _check_asset_available(self, release: None | Version): releases = { k: v for k, v in releases.items() if not k.is_prerelease } - logger.debug(f"Available versions: {sorted(releases)}") + logger.debug(f"Available releases: {sorted(releases)}") latest_release = max(releases) if releases else CURRENT_VERSION update_available = CURRENT_VERSION < latest_release @@ -245,25 +297,30 @@ def _check_asset_available(self, release: None | Version): # If the asset is not available, then check the next latest # release, and so on until either a new asset is available or there # is no update available. - asset_available = False - while update_available and not asset_available: + while update_available: asset_info = get_asset_info(releases.get(latest_release)) if asset_info is not None: - # The asset is available - logger.debug(f"Asset available for url: {asset_info['url']}") - asset_available = True - else: - # The asset is not available - logger.debug(f"Asset not available: {latest_release}") - asset_available = False - releases.pop(latest_release) + # The asset is available, now get checksum. + checksum = get_asset_checksum( + asset_info["checksum_url"], asset_info["filename"] + ) + if checksum is not None: + asset_info.update(checksum=checksum) + logger.debug(f"Asset available: {latest_release}") + break + else: + asset_info = None - latest_release = max(releases) if releases else CURRENT_VERSION - update_available = CURRENT_VERSION < latest_release + # The asset is not available + logger.debug(f"Asset not available: {latest_release}") + releases.pop(latest_release) - logger.debug(f"Latest release: {latest_release}") - logger.debug(f"Update available: {update_available}") + latest_release = max(releases) if releases else CURRENT_VERSION + update_available = CURRENT_VERSION < latest_release + + logger.debug(f"Latest release: {latest_release}") + logger.debug(f"Update available: {update_available}") self.asset_info = asset_info @@ -381,12 +438,6 @@ def _download_installer(self): r.raise_for_status() size = int(r.headers["content-length"]) - exp_size = self.asset_info["size"] - if exp_size != size: - raise UpdateDownloadIncompleteError( - f"Download error: size = {size} bytes; " - f"expected {exp_size} bytes" - ) self._progress_reporter(0, size) with open(self.installer_path, 'wb') as f: @@ -397,17 +448,14 @@ def _download_installer(self): f.write(chunk) self._progress_reporter(size_read, size) - if size_read == exp_size: + if validate_download(self.installer_path, self.asset_info["checksum"]): logger.info('Download successfully completed.') if self.installer_path.endswith('.zip'): with ZipFile(self.installer_path, 'r') as f: f.extractall(dirname) else: - raise UpdateDownloadIncompleteError( - "Download incomplete: retrieved only " - f"{size_read} out of {size} bytes." - ) + raise UpdateDownloadError("Download failed!") def _clean_installer_dir(self): """Remove downloaded file""" From bd13a22968185bb2be7f08e40c5e4b5c285892dd Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Thu, 3 Apr 2025 07:08:50 -0700 Subject: [PATCH 5/8] Use 6.0.0rc1 instead of 6.0.0b3 because the latter does not have the correct assets --- spyder/plugins/updatemanager/tests/test_update_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index 1db2662cbf2..a8969b7cd97 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -69,7 +69,7 @@ def test_updates(qtbot, mocker, caplog, version, channel): @pytest.mark.parametrize("version", ["4.0.0a1", "4.0.0"]) -@pytest.mark.parametrize("release", ["6.0.0", "6.0.0b3"]) +@pytest.mark.parametrize("release", ["6.0.0", "6.0.0rc1"]) @pytest.mark.parametrize("stable_only", [True, False]) def test_update_non_stable(qtbot, mocker, version, release, stable_only): """Test we offer unstable updates.""" From cab10bb235e6ee95ba43c682aa456d4fbd9df675 Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Thu, 3 Apr 2025 16:14:25 -0700 Subject: [PATCH 6/8] Retrieve prescribed set of releases from Github for unit testing purposes --- .../tests/test_update_manager.py | 8 ++++ spyder/plugins/updatemanager/workers.py | 41 +++++++++++++++---- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index a8969b7cd97..c951c10a7f2 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -23,6 +23,14 @@ workers.get_github_releases = lru_cache(workers.get_github_releases) workers.get_asset_checksum = lru_cache(workers.get_asset_checksum) +_tags = ( + "v6.0.5", "v6.0.5rc1", "v6.1.0a1", "v6.0.4", + "v6.0.4rc1", "v6.0.3", "v6.0.3rc2", "v6.0.3rc1", + "v6.0.2", "v6.0.2rc1", "v6.0.1", "v6.0.0", + "v5.5.6", "v6.0.0rc2", "v6.0.0rc1", "v6.0.0b3", + "v6.0.0b2", "v5.5.5", "v6.0.0b1", "v6.0.0a5" +) +workers.get_github_releases(_tags) # Run once to cache result for tests @pytest.fixture(autouse=True) diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index 4206ab01583..fec7fc7f885 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -94,23 +94,46 @@ class AssetInfo(TypedDict): checksum: str -def get_github_releases() -> dict[Version, dict]: +def get_github_releases( + tags: str | tuple[str] | None = None +) -> dict[Version, dict]: """ Get Github release information + Parameters + ---------- + tags : str | tuple[str] | (None) + If tags is provided, only release information for the requeste tags + is retrieved. Otherwise, the most recent 20 releases are retrieved. + This is only used to retrieve a known set of releases for unit testing. + Returns ------- releases : dict[packaging.version.Version, dict] Dictionary of release information. """ - url = ( - "https://api.github.com/repos/spyder-ide/spyder/releases" - "?per_page=20&page=1" - ) - logger.info(f"Getting release info from {url}") - page = requests.get(url, headers=GH_HEADERS) - page.raise_for_status() - data = page.json() + url = "https://api.github.com/repos/spyder-ide/spyder/releases" + if tags is None: + # Get 20 most recent releases + url += "?per_page=20&page=1" + logger.info(f"Getting release info from {url}") + page = requests.get(url, headers=GH_HEADERS) + page.raise_for_status() + data = page.json() + else: + # Get specified releases + tags = [tags] if isinstance(tags, str) else tags + url += "/tags/" + + data = [] + with requests.Session() as session: + session.headers = GH_HEADERS + logger.info(f"Getting release info for {tags}") + for tag in tags: + _url = url + tag + page = session.get(_url) + page.raise_for_status() + data.append(page.json()) return {parse(item['tag_name']): item for item in data} From 261d4111a6d3617f5c882376317ff8eda983feb8 Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Sat, 5 Apr 2025 11:55:39 -0700 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Carlos Cordoba --- .../tests/test_update_manager.py | 7 +- spyder/plugins/updatemanager/workers.py | 74 +++++++++++++++---- 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/spyder/plugins/updatemanager/tests/test_update_manager.py b/spyder/plugins/updatemanager/tests/test_update_manager.py index c951c10a7f2..1b9dee5def6 100644 --- a/spyder/plugins/updatemanager/tests/test_update_manager.py +++ b/spyder/plugins/updatemanager/tests/test_update_manager.py @@ -85,7 +85,7 @@ def test_update_non_stable(qtbot, mocker, version, release, stable_only): release = parse(release) worker = WorkerUpdate(stable_only) - worker._check_asset_available(release) + worker._check_update_available(release) if release.is_prerelease and stable_only: assert worker.asset_info is None @@ -101,7 +101,7 @@ def test_update_no_asset(qtbot, mocker, version, release): release = parse(release) if release else None worker = WorkerUpdate(True) - worker._check_asset_available(release) + worker._check_update_available(release) # For both values of version, there should be an update available # However, the available version should be 6.0.1, since there is @@ -126,8 +126,7 @@ def test_get_asset_info(qtbot, mocker, app, version, release, update_type): mocker.patch.object(workers, "is_conda_based_app", return_value=app) worker = WorkerUpdate(False) - # import pdb; pdb.set_trace() - worker._check_asset_available(parse(release)) + worker._check_update_available(parse(release)) info = worker.asset_info assert info['update_type'] == update_type diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index fec7fc7f885..07c67410bff 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -103,7 +103,7 @@ def get_github_releases( Parameters ---------- tags : str | tuple[str] | (None) - If tags is provided, only release information for the requeste tags + If tags is provided, only release information for the requested tags is retrieved. Otherwise, the most recent 20 releases are retrieved. This is only used to retrieve a known set of releases for unit testing. @@ -113,6 +113,7 @@ def get_github_releases( Dictionary of release information. """ url = "https://api.github.com/repos/spyder-ide/spyder/releases" + if tags is None: # Get 20 most recent releases url += "?per_page=20&page=1" @@ -224,7 +225,24 @@ def get_asset_info(release_info: dict) -> None | AssetInfo: return asset_info -def validate_download(file, checksum): +def validate_download(file: str, checksum: str) -> bool: + """ + Compute the sha256 checksum of the provided file and compare against + the provided checksum. + + Parameters + ---------- + file : str + Full path to the file to be verified. + checksum : str + sha256 checksum to match against the computed checksum. + + Returns + ------- + valid : bool + True if the file's computed checksum matches the provided checksum, + False otherwise. + """ with open(file, "rb") as f: _checksum = sha256() while chunk := f.read(8192): @@ -290,20 +308,32 @@ def __init__(self, stable_only): self.asset_info = None self.error = None self.checkbox = False - self.channel = None - def _check_asset_available(self, release: None | Version): - """Checks if there is an asset available for release.""" + def _check_update_available(self, release: Version | None = None): + """ + Check if there is an update available. + + Releases are obtained from Github and compared to the current Spyder + version to determine if an update is available. The Github release + asset and checksum must be available in order for the release to be + considered. + + Parameters + ---------- + release : packaging.version.Version | (None) + If provided, limit possible releases on Github to this release + or less. + """ # Get asset info from Github releases = get_github_releases() if release is not None: - # Only keep Github releases that are also available at channel url + # Limit Github releases to consider releases = {k: v for k, v in releases.items() if k <= release} if self.stable_only: - # Only use stable releases + # Only consider stable releases releases = { k: v for k, v in releases.items() if not k.is_prerelease } @@ -355,26 +385,38 @@ def start(self): url = None if not is_conda_based_app(): - self.channel = "pypi" # Default channel if not conda if is_conda_env(sys.prefix): - self.channel, channel_url = get_spyder_conda_channel() - url = channel_url + '/channeldata.json' + channel, url = get_spyder_conda_channel() + url += '/channeldata.json' + else: + channel = "pypi" + url = "https://pypi.python.org/pypi/spyder/json" try: release = None if url is not None: + # Limit the releases on Github that we consider to those less + # than or equal to what is also available on the conda/pypi + # channel + logger.info(f"Getting release from {url}") page = requests.get(url) page.raise_for_status() data = page.json() - # Conda pkgs/main or conda-forge url - # What about PyPi? - spyder_data = data['packages'].get('spyder') - if spyder_data: - release = parse(spyder_data["version"]) + if channel == "pypi": + releases = [ + parse(k) for k in data["releases"].keys() + if not parse(k).is_prerelease + ] + release = max(releases) + else: + # Conda pkgs/main or conda-forge url + spyder_data = data['packages'].get('spyder') + if spyder_data: + release = parse(spyder_data["version"]) - self._check_asset_available(release) + self._check_update_available(release) except SSLError as err: error_msg = SSL_ERROR_MSG From e692d11129793e750c1adadeb1bc5e01d57a44be Mon Sep 17 00:00:00 2001 From: Ryan Clary <9618975+mrclary@users.noreply.github.com> Date: Sun, 6 Apr 2025 10:21:58 -0700 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Carlos Cordoba --- spyder/plugins/updatemanager/workers.py | 32 +++++++++++-------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/spyder/plugins/updatemanager/workers.py b/spyder/plugins/updatemanager/workers.py index 07c67410bff..09675455c69 100644 --- a/spyder/plugins/updatemanager/workers.py +++ b/spyder/plugins/updatemanager/workers.py @@ -169,7 +169,7 @@ def get_asset_checksum(url: str, name: str) -> str | None: return checksum -def get_asset_info(release_info: dict) -> None | AssetInfo: +def get_asset_info(release_info: dict) -> AssetInfo | None: """ Get the version, name, update type, download URL, and size for the asset of the given release. @@ -181,7 +181,7 @@ def get_asset_info(release_info: dict) -> None | AssetInfo: Returns ------- - asset_info: None | AssetInfo + asset_info: AssetInfo | None Information about the asset. """ release = parse(release_info["tag_name"]) @@ -308,6 +308,7 @@ def __init__(self, stable_only): self.asset_info = None self.error = None self.checkbox = False + self.channel = None def _check_update_available(self, release: Version | None = None): """ @@ -379,18 +380,16 @@ def _check_update_available(self, release: Version | None = None): def start(self): """Main method of the worker.""" - self.error = None - self.asset_info = None - error_msg = None - url = None if not is_conda_based_app(): + self.channel = "pypi" # Default channel if not conda if is_conda_env(sys.prefix): - channel, url = get_spyder_conda_channel() - url += '/channeldata.json' - else: - channel = "pypi" + self.channel, url = get_spyder_conda_channel() + + if self.channel == "pypi": url = "https://pypi.python.org/pypi/spyder/json" + else: + url += '/channeldata.json' try: release = None @@ -398,13 +397,12 @@ def start(self): # Limit the releases on Github that we consider to those less # than or equal to what is also available on the conda/pypi # channel - logger.info(f"Getting release from {url}") page = requests.get(url) page.raise_for_status() data = page.json() - if channel == "pypi": + if self.channel == "pypi": releases = [ parse(k) for k in data["releases"].keys() if not parse(k).is_prerelease @@ -419,17 +417,17 @@ def start(self): self._check_update_available(release) except SSLError as err: - error_msg = SSL_ERROR_MSG + self.error = SSL_ERROR_MSG logger.warning(err, exc_info=err) except ConnectionError as err: - error_msg = CONNECT_ERROR_MSG + self.error = CONNECT_ERROR_MSG logger.warning(err, exc_info=err) except HTTPError as err: status_code = err.response.status_code - error_msg = HTTP_ERROR_MSG.format(status_code=status_code) + self.error = HTTP_ERROR_MSG.format(status_code=status_code) logger.warning(err, exc_info=err) except OSError as err: - error_msg = OS_ERROR_MSG.format(error=err) + self.error = OS_ERROR_MSG.format(error=err) self.checkbox = True logger.warning(err, exc_info=err) except Exception as err: @@ -442,8 +440,6 @@ def start(self): self.sig_exception_occurred.emit(error_data) logger.error(err, exc_info=err) finally: - self.error = error_msg - # At this point we **must** emit the signal below so that the # "Check for updates" action in the Help menu is enabled again # after the check has finished (it's disabled while the check is