launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+488593@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/charm/launchpad-native-publisher/reactive/launchpad-native-publisher.py b/charm/launchpad-native-publisher/reactive/launchpad-native-publisher.py
> index 63f7999..52b3f70 100644
> --- a/charm/launchpad-native-publisher/reactive/launchpad-native-publisher.py
> +++ b/charm/launchpad-native-publisher/reactive/launchpad-native-publisher.py
> @@ -90,12 +89,112 @@ def perform_action_on_services(services, action):
>          action(service)
>  
>  
> +def check_binaries_exist():
> +    """Check if required binaries exist and are executable."""
> +    required_binaries = ["java", "mvn", "rustc", "cargo"]
> +    missing = []
> +
> +    for binary in required_binaries:
> +        try:
> +            subprocess.check_call(
> +                ["which", binary],
> +                stderr=subprocess.DEVNULL,
> +                stdout=subprocess.DEVNULL,
> +            )
> +        except subprocess.CalledProcessError:
> +            missing.append(binary)
> +            hookenv.log(f"{binary} is missing")
> +
> +    return missing
> +
> +
> +def apt_install(*packages):
> +    """Install packages via apt."""
> +    subprocess.check_call(["apt-get", "install", "-y"] + list(packages))
> +
> +
> +def update_alternatives(name, path, priority="100"):
> +    """Set up update-alternatives for a binary."""
> +    subprocess.check_call(
> +        [
> +            "update-alternatives",
> +            "--install",
> +            f"/usr/bin/{name}",
> +            name,
> +            path,
> +            priority,
> +        ]
> +    )
> +
> +
> +def install_dependencies():
> +    """Install Java and Rust dependencies."""
> +    config = hookenv.config()
> +    java_version = config.get("java_version")
> +    rust_version = config.get("rust_version")
> +
> +    missing = check_binaries_exist()
> +    missing_java = any(binary in missing for binary in ["java", "mvn"])
> +    missing_rust = any(binary in missing for binary in ["rustc", "cargo"])
> +
> +    try:
> +        # Install Java and Maven only if needed
> +        if missing_java:

I think we don't actually need to check if it's missing, we can install it anyway, it'll install the latest security upgrades.

> +            subprocess.check_call(["apt-get", "update"])
> +            hookenv.log("Installing Java packages...")
> +            apt_install(
> +                f"openjdk-{java_version}-jdk",
> +                "maven",
> +                "libmaven-deploy-plugin-java",
> +            )
> +
> +        # Install Rust and Cargo only if needed
> +        if missing_rust:
> +            subprocess.check_call(["apt-get", "update"])
> +            hookenv.log("Installing Rust packages...")
> +            apt_install(f"rustc-{rust_version}", f"cargo-{rust_version}")
> +
> +            # Use update-alternatives to create generic command names
> +            hookenv.log("Setting up alternatives for Rust and Cargo...")
> +            update_alternatives("rustc", f"/usr/bin/rustc-{rust_version}")
> +            update_alternatives("cargo", f"/usr/bin/cargo-{rust_version}")
> +
> +        # Verify installations
> +        hookenv.log("Verifying installations...")
> +        missing = check_binaries_exist()
> +        if missing:
> +            raise RuntimeError(
> +                f"Still missing binaries after installation: {missing}"
> +            )
> +
> +        hookenv.log("All dependencies successfully installed and verified")
> +
> +    except subprocess.CalledProcessError as e:
> +        hookenv.log(f"Failed to install dependencies: {e}", level="ERROR")
> +        raise
> +    except RuntimeError as e:
> +        hookenv.log(str(e), level="ERROR")
> +        raise
> +
> +
> +def ensure_dependencies():
> +    """Ensure required dependencies are installed."""
> +    hookenv.log("Checking if binaries are already installed...")
> +    missing = check_binaries_exist()
> +    if missing:

here as well, I think we can just install anyway, we don't need to check if it's missing, we just need the missing check at the end

> +        install_dependencies()
> +    else:
> +        hookenv.log("All required binaries are available")
> +
> +
>  @when(
>      "launchpad.db.configured",
>  )
>  @when_not("service.configured")
>  def configure():
>      """Configure the native publisher service."""
> +    ensure_dependencies()
> +
>      config = get_service_config()
>      try:
>          if config.get("craftbuild_config"):


-- 
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/488593
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:native-pub-ensure-deps into launchpad:master.