Skip to content
Open
108 changes: 78 additions & 30 deletions conan/internal/runner/ssh.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from pathlib import Path
from typing import Iterable
import fnmatch
import pathlib
import tempfile

Expand All @@ -9,6 +11,8 @@
from io import BytesIO
import sys

from conan.internal.runner.output import RunnerOutput

def ssh_info(msg, error=False):
fg=Color.BRIGHT_MAGENTA
if error:
Expand Down Expand Up @@ -48,6 +52,8 @@ def __init__(self, conan_api, command, host_profile, build_profile, args, raw_ar
self.client = SSHClient()
self.client.load_system_host_keys()
self.client.connect(hostname)
self.runner_output = RunnerOutput(hostname)
self.remote_conn = RemoteConnection(self.client, self.runner_output)
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to have both a self.client that is a SSHClient, and to have another self.remote_conn, that is an object that contains another SSHClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern and agree with you.
self.client will be removed in a future PR. I'm trying to split changes from #17357 and to do the migration in an incremental way, some duplications have to coexist.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with it, maybe just add a couple of comments:

# this self.client manages the main ssh connection
self.client = SSHClient()

# This client manages just the sftp transfers
# TODO: Integrate both client calls in one
self.remote_conn = RemoteConnection

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern and agree with you.
self.client will be removed in a future PR. I'm trying to split changes from #17357 and to do the migration in an incremental way, some duplications have to coexist.

I'm not entirely sure it's justified to have a whole class just to wrap simple calls - however, I would argue that these are not duplicates, just have very confusing names.

self.client = SSHClient() this is very clear

self.remote_conn = RemoteConnection this not so much - this is only wrapping around paramiko's SFTPClient - which is a different class because it's a different connection. Arguably, the RemoteConnection class does not need to get a new one for each operation.

Could be two attributes, self.ssh_client and self.sftp_client - persist the latter throughout the entire execution, rather than open a new connection for each operation (see my comments elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, I've updated the code renaming the class properties



def run(self, use_cache=True):
Expand Down Expand Up @@ -138,14 +144,7 @@ def ensure_runner_environment(self):
ssh_info(f"Expected remote conan command: {conan_cmd}")

# Check if remote Conan executable exists, otherwise invoke pip inside venv
sftp = self.client.open_sftp()
try:
sftp.stat(conan_cmd)
has_remote_conan = True
except FileNotFoundError:
has_remote_conan = False
finally:
sftp.close()
has_remote_conan = self.remote_conn.check_file_exists(conan_cmd)

if not has_remote_conan:
_, _stdout, _stderr = self.client.exec_command(f"{python_command} -m venv {conan_venv}")
Expand Down Expand Up @@ -183,26 +182,15 @@ def ensure_runner_environment(self):
_, _stdout, _stderr = self.client.exec_command(f"{self.remote_conan} config home")
ssh_info(f"Remote conan config home returned: {_stdout.read().decode().strip()}")
_, _stdout, _stderr = self.client.exec_command(f"{self.remote_conan} profile detect --force")
self._copy_profiles()


def _copy_profiles(self):
sftp = self.client.open_sftp()
self._sync_conan_config()

# TODO: very questionable choices here
try:
profiles = {
self.args.profile_host[0]: self.host_profile.dumps(),
self.args.profile_build[0]: self.build_profile.dumps()
}

for name, contents in profiles.items():
dest_filename = self.remote_conan_home + f"/profiles/{name}"
sftp.putfo(BytesIO(contents.encode()), dest_filename)
except:
raise ConanException("Unable to copy profiles to remote")
finally:
sftp.close()
def _sync_conan_config(self):
# Transfer conan config to remote
self.remote_conn.put_dir(
self.conan_api.config.home(),
Copy link
Contributor

Choose a reason for hiding this comment

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

What folder is this sending, exactly? I would probably have something more explicit rather than anything by exclusion -
We could stand simple and just sync the profiles folder (maybe the remotes and maybe global.conf)

we should probably think ahead - there's a chance we'll have to be more selective (any configs that have paths that are relevant to the "local" machine will not work on the remote runner!)

Copy link
Contributor Author

@perseoGI perseoGI Jul 23, 2025

Choose a reason for hiding this comment

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

That is a good approach. I've updated the method to only copy:

  • profiles folder
  • settings.yml
  • settings_user.yml
  • global.conf
  • remotes.json

I've added a TODO note to check for local path references on each file before copying. If you agree, I can implement a basic version of that functionality in this PR, or I can leave it for a future (and isolated) PR.

self.remote_conan_home,
exclude_patterns=["p", ".conan.db", "*.pyc", "__pycache__", ".DS_Store", ".git"]
)

def copy_working_conanfile_path(self):
resolved_path = Path(self.args.path).resolve()
Expand All @@ -220,6 +208,7 @@ def copy_working_conanfile_path(self):
self.remote_create_dir = _stdout.read().decode().strip().replace("\\", '/')

# Copy current folder to destination using sftp
# self.remote_conn.put_dir(resolved_path.as_posix(), self.remote_create_dir)
_Path = pathlib.PureWindowsPath if self.remote_is_windows else pathlib.PurePath
sftp = self.client.open_sftp()
for root, dirs, files in os.walk(resolved_path.as_posix()):
Expand Down Expand Up @@ -265,8 +254,67 @@ def update_local_cache(self, json_result):
if stdout.channel.recv_exit_status() != 0:
raise ConanException("Unable to save remote conan cache state")

sftp = self.client.open_sftp()
with tempfile.TemporaryDirectory() as tmp:
local_cache_tgz = os.path.join(tmp, 'cache.tgz')
sftp.get(conan_cache_tgz, local_cache_tgz)
package_list = self.conan_api.cache.restore(local_cache_tgz)
self.remote_conn.get(conan_cache_tgz, local_cache_tgz)
self.conan_api.cache.restore(local_cache_tgz)


class RemoteConnection:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to allow Python annotations in this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot answer this to you, IMHO I prefer annotations

def __init__(self, client, runner_output: RunnerOutput):
from paramiko.client import SSHClient
self.client: SSHClient = client
self.runner_output = runner_output

def put(self, src: str, dst: str) -> None:
try:
sftp = self.client.open_sftp()
sftp.put(src, dst)
sftp.close()
except IOError as e:
self.runner_output.error(f"Unable to copy {src} to {dst}:\n{e}")

def put_dir(self, src: str, dst: str, exclude_patterns: Iterable[str] = []) -> None:
source_folder = Path(src)
destination_folder = Path(dst)
for item in source_folder.iterdir():
dest_item = (destination_folder / item.name).as_posix()
# Check if item matches any exclude pattern
if any(fnmatch.fnmatch(item.name, pattern) for pattern in exclude_patterns):
continue
if item.is_file():
self.runner_output.verbose(f"Copying file {item.as_posix()} to {dest_item}")
self.put(item.as_posix(), dest_item)
Copy link
Contributor

Choose a reason for hiding this comment

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

we are opening a new sftp connection for each file, which doesn't feel right, i would open/close a new connection for every single file operation.
I would rename this class sftpClient or something to that effect, keep the paramiko object as an attribute (passed to the constructor), and persist it instead of calling open_sftp() from this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good improve in performance, thank you!

elif item.is_dir():
self.runner_output.verbose(f"Copying directory {item.as_posix()} to {dest_item}")
self.mkdir(dest_item, ignore_existing=True)
self.put_dir(item.as_posix(), dest_item, exclude_patterns)

def get(self, src: str, dst: str) -> None:
try:
sftp = self.client.open_sftp()
sftp.get(src, dst)
sftp.close()
except IOError as e:
self.runner_output.error(f"Unable to copy from remote {src} to {dst}:\n{e}")

def mkdir(self, folder: str, ignore_existing=False) -> None:
sftp = self.client.open_sftp()
try:
sftp.mkdir(folder)
except IOError:
if ignore_existing:
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible for the IOError exception to be raised by something else other than the directory existing??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in the context of mkdir

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the error type different if you don't have permission to create the folder? The question is if you can hide a permissions error if you have ignore_existing=True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code with a more exhaustive exception check, thanks!

pass
else:
raise
finally:
sftp.close()

def check_file_exists(self, file: str) -> bool:
try:
sftp = self.client.open_sftp()
sftp.stat(file)
sftp.close()
return True
except FileNotFoundError:
return False
Loading