From 28029c25c0bc6b7a23bff925f3af6f012877eb31 Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Sun, 8 Dec 2024 16:27:58 -0800 Subject: [PATCH] fix: Some Python improvements. (#1748) * Fix venv. * Fix venv paths. * Polish. * Change reqs. --- CHANGELOG.md | 15 +++++- crates/app/src/commands/ci.rs | 2 +- crates/cli/tests/run_python_test.rs | 4 ++ ...thon_test__runs_install_deps_via_args.snap | 4 +- crates/config/src/toolchain/python_config.rs | 1 - crates/toolchain/src/detect/languages.rs | 1 + crates/toolchain/src/detect/task_platform.rs | 2 +- .../platform/src/actions/install_deps.rs | 54 +++++++++---------- legacy/python/platform/src/lib.rs | 7 --- legacy/python/platform/src/python_platform.rs | 16 +++--- legacy/python/tool/src/python_tool.rs | 26 ++++++--- tests/fixtures/python/base/.gitignore | 1 + website/docs/config/toolchain.mdx | 7 ++- 13 files changed, 79 insertions(+), 61 deletions(-) create mode 100644 tests/fixtures/python/base/.gitignore diff --git a/CHANGELOG.md b/CHANGELOG.md index 50411366e7e..921e480e7c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,23 @@ # Changelog +## Unreleased + +#### 🐞 Fixes + +- Fixed `moon ci` showing incorrect job related logs. +- Fixed some issues with the Python toolchain: + - pip is no longer required to be enabled to activate a virtual environment. + - Changed `python.rootRequirementsOnly` to `false` by default. + - The venv root is now the location of a found `requirements.txt`, otherwise the package root, or + workspace root if `python.rootRequirementsOnly` is enabled. + - Tasks will now inherit the correct venv paths in `PATH`. + ## 1.30.3 #### 🐞 Fixes -- Fixed an issue where a task with explicit no inputs (`inputs: []`) would always be marked as affected. +- Fixed an issue where a task with explicit no inputs (`inputs: []`) would always be marked as + affected. #### ⚙️ Internal diff --git a/crates/app/src/commands/ci.rs b/crates/app/src/commands/ci.rs index 297bde9508e..a3f48cf1c5e 100644 --- a/crates/app/src/commands/ci.rs +++ b/crates/app/src/commands/ci.rs @@ -175,7 +175,7 @@ fn distribute_targets_across_jobs( let job_index = args.job.unwrap_or_default(); let job_total = args.job_total.unwrap_or_default(); - let batch_size = (targets.len() + job_total - 1) / job_total; + let batch_size = targets.len().div_ceil(job_total); let batched_targets; console.print_header("Distributing targets across jobs")?; diff --git a/crates/cli/tests/run_python_test.rs b/crates/cli/tests/run_python_test.rs index d805e17198c..f3367cda688 100644 --- a/crates/cli/tests/run_python_test.rs +++ b/crates/cli/tests/run_python_test.rs @@ -57,6 +57,10 @@ fn runs_install_deps_via_args() { }), ..PartialPythonConfig::default() }); + + // Needed for venv + sandbox.create_file("base/requirements.txt", ""); + let assert = sandbox.run_moon(|cmd| { cmd.arg("run").arg("python:poetry"); }); diff --git a/crates/cli/tests/snapshots/run_python_test__runs_install_deps_via_args.snap b/crates/cli/tests/snapshots/run_python_test__runs_install_deps_via_args.snap index 4fa7044b750..76b1ba8a347 100644 --- a/crates/cli/tests/snapshots/run_python_test__runs_install_deps_via_args.snap +++ b/crates/cli/tests/snapshots/run_python_test__runs_install_deps_via_args.snap @@ -1,10 +1,8 @@ --- source: crates/cli/tests/run_python_test.rs -assertion_line: 60 expression: assert.output() -snapshot_kind: text --- -▪▪▪▪ activate virtual environment +▪▪▪▪ python venv ▪▪▪▪ pip install ▪▪▪▪ python:poetry Poetry (version 1.8.4) diff --git a/crates/config/src/toolchain/python_config.rs b/crates/config/src/toolchain/python_config.rs index 9c7e2e4cab4..30b7c21b7af 100644 --- a/crates/config/src/toolchain/python_config.rs +++ b/crates/config/src/toolchain/python_config.rs @@ -21,7 +21,6 @@ pub struct PythonConfig { /// Assumes only the root `requirements.txt` is used for dependencies. /// Can be used to support the "one version policy" pattern. - #[setting(default = true)] pub root_requirements_only: bool, /// Defines the virtual environment name, which will be created in the workspace root. diff --git a/crates/toolchain/src/detect/languages.rs b/crates/toolchain/src/detect/languages.rs index 15d2781b7ee..a31cab8df70 100644 --- a/crates/toolchain/src/detect/languages.rs +++ b/crates/toolchain/src/detect/languages.rs @@ -38,6 +38,7 @@ pub static PYTHON: StaticStringList = &[ "pyproject.toml", ".pylock.toml", ".python-version", + ".venv", // pip "Pipfile", "Pipfile.lock", diff --git a/crates/toolchain/src/detect/task_platform.rs b/crates/toolchain/src/detect/task_platform.rs index d294942854d..60de6b03beb 100644 --- a/crates/toolchain/src/detect/task_platform.rs +++ b/crates/toolchain/src/detect/task_platform.rs @@ -60,7 +60,7 @@ pub fn detect_task_platform(command: &str, enabled_platforms: &[PlatformType]) - } if PYTHON_COMMANDS - .get_or_init(|| Regex::new("^(python|python3|pip|pip3)$").unwrap()) + .get_or_init(|| Regex::new("^(python|python3|python-3|pip|pip3|pip-3)$").unwrap()) .is_match(command) { return use_platform_if_enabled(PlatformType::Python, enabled_platforms); diff --git a/legacy/python/platform/src/actions/install_deps.rs b/legacy/python/platform/src/actions/install_deps.rs index 7984616af11..981c08f40bf 100644 --- a/legacy/python/platform/src/actions/install_deps.rs +++ b/legacy/python/platform/src/actions/install_deps.rs @@ -1,10 +1,8 @@ use moon_action::Operation; use moon_console::{Checkpoint, Console}; -use moon_python_tool::PythonTool; +use moon_python_tool::{find_requirements_txt, PythonTool}; use std::path::Path; -use crate::find_requirements_txt; - pub async fn install_deps( python: &PythonTool, workspace_root: &Path, @@ -12,33 +10,33 @@ pub async fn install_deps( console: &Console, ) -> miette::Result> { let mut operations = vec![]; + let requirements_path = find_requirements_txt(working_dir, workspace_root); - if let Some(pip_config) = &python.config.pip { - let requirements_path = find_requirements_txt(working_dir, workspace_root); - let virtual_environment = if python.config.root_requirements_only { - workspace_root.join(&python.config.venv_name) - } else { - working_dir.join(&python.config.venv_name) - }; + let venv_root = if python.config.root_requirements_only { + workspace_root.join(&python.config.venv_name) + } else { + requirements_path + .as_ref() + .and_then(|rp| rp.parent()) + .unwrap_or(working_dir) + .join(&python.config.venv_name) + }; - if !virtual_environment.exists() { - console - .out - .print_checkpoint(Checkpoint::Setup, "activate virtual environment")?; + if !venv_root.exists() && requirements_path.is_some() { + console + .out + .print_checkpoint(Checkpoint::Setup, "python venv")?; - let args = vec![ - "-m", - "venv", - virtual_environment.to_str().unwrap_or_default(), - ]; + let args = vec!["-m", "venv", venv_root.to_str().unwrap_or_default()]; - operations.push( - Operation::task_execution(format!("python {}", args.join(" "))) - .track_async(|| python.exec_python(args, workspace_root)) - .await?, - ); - } + operations.push( + Operation::task_execution(format!("python {}", args.join(" "))) + .track_async(|| python.exec_python(args, working_dir, workspace_root)) + .await?, + ); + } + if let Some(pip_config) = &python.config.pip { let mut args = vec![]; // Add pip installArgs, if users have given @@ -47,8 +45,8 @@ pub async fn install_deps( } // Add requirements.txt path, if found - if let Some(req) = &requirements_path { - args.extend(["-r", req.to_str().unwrap_or_default()]); + if let Some(reqs_path) = requirements_path.as_ref().and_then(|req| req.to_str()) { + args.extend(["-r", reqs_path]); } if !args.is_empty() { @@ -60,7 +58,7 @@ pub async fn install_deps( operations.push( Operation::task_execution(format!("python {}", args.join(" "))) - .track_async(|| python.exec_python(args, working_dir)) + .track_async(|| python.exec_python(args, working_dir, workspace_root)) .await?, ); } diff --git a/legacy/python/platform/src/lib.rs b/legacy/python/platform/src/lib.rs index 3e1a597bf12..1b0dbc7911d 100644 --- a/legacy/python/platform/src/lib.rs +++ b/legacy/python/platform/src/lib.rs @@ -3,10 +3,3 @@ mod python_platform; mod toolchain_hash; pub use python_platform::*; - -use starbase_utils::fs; -use std::path::{Path, PathBuf}; - -fn find_requirements_txt(starting_dir: &Path, workspace_root: &Path) -> Option { - fs::find_upwards_until("requirements.txt", starting_dir, workspace_root) -} diff --git a/legacy/python/platform/src/python_platform.rs b/legacy/python/platform/src/python_platform.rs index d276919260a..1d4a93113d8 100644 --- a/legacy/python/platform/src/python_platform.rs +++ b/legacy/python/platform/src/python_platform.rs @@ -1,4 +1,4 @@ -use crate::{actions, find_requirements_txt, toolchain_hash::PythonToolchainHash}; +use crate::{actions, toolchain_hash::PythonToolchainHash}; use moon_action::Operation; use moon_action_context::ActionContext; use moon_common::{path::is_root_level_source, Id}; @@ -12,7 +12,7 @@ use moon_platform::{Platform, Runtime, RuntimeReq}; use moon_process::Command; use moon_project::Project; use moon_python_lang::pip_requirements::load_lockfile_dependencies; -use moon_python_tool::{get_python_tool_paths, PythonTool}; +use moon_python_tool::{find_requirements_txt, get_python_tool_paths, PythonTool}; use moon_task::Task; use moon_tool::{get_proto_version_env, prepend_path_env_var, Tool, ToolManager}; use moon_utils::async_trait; @@ -294,16 +294,14 @@ impl Platform for PythonPlatform { if let Ok(python) = self.toolchain.get_for_version(&runtime.requirement) { if let Some(version) = get_proto_version_env(&python.tool) { - let cwd = if python.config.root_requirements_only { - self.workspace_root.as_path() - } else { - working_dir - }; - command.env("PROTO_PYTHON_VERSION", version); command.env( "PATH", - prepend_path_env_var(get_python_tool_paths(python, cwd)), + prepend_path_env_var(get_python_tool_paths( + python, + working_dir, + &self.workspace_root, + )), ); } } diff --git a/legacy/python/tool/src/python_tool.rs b/legacy/python/tool/src/python_tool.rs index 28dcaff599c..f663932d996 100644 --- a/legacy/python/tool/src/python_tool.rs +++ b/legacy/python/tool/src/python_tool.rs @@ -10,16 +10,25 @@ use moon_toolchain::RuntimeReq; use proto_core::flow::install::InstallOptions; use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool, UnresolvedVersionSpec}; use rustc_hash::FxHashMap; +use starbase_utils::fs; use std::path::PathBuf; use std::sync::Arc; use std::{ffi::OsStr, path::Path}; use tracing::instrument; -pub fn get_python_tool_paths(python_tool: &PythonTool, working_dir: &Path) -> Vec { - let venv_python = working_dir.join(&python_tool.config.venv_name); +pub fn find_requirements_txt(starting_dir: &Path, workspace_root: &Path) -> Option { + fs::find_upwards_until("requirements.txt", starting_dir, workspace_root) +} - if venv_python.exists() { - vec![venv_python.join("Scripts"), venv_python.join("bin")] +pub fn get_python_tool_paths( + python_tool: &PythonTool, + working_dir: &Path, + workspace_root: &Path, +) -> Vec { + if let Some(venv_root) = + fs::find_upwards_until(&python_tool.config.venv_name, working_dir, workspace_root) + { + vec![venv_root.join("Scripts"), venv_root.join("bin")] } else { get_proto_paths(&python_tool.proto_env) } @@ -68,7 +77,12 @@ impl PythonTool { } #[instrument(skip_all)] - pub async fn exec_python(&self, args: I, working_dir: &Path) -> miette::Result<()> + pub async fn exec_python( + &self, + args: I, + working_dir: &Path, + workspace_root: &Path, + ) -> miette::Result<()> where I: IntoIterator, S: AsRef, @@ -78,7 +92,7 @@ impl PythonTool { .envs(get_proto_env_vars()) .env( "PATH", - prepend_path_env_var(get_python_tool_paths(self, working_dir)), + prepend_path_env_var(get_python_tool_paths(self, working_dir, workspace_root)), ) .cwd(working_dir) .with_console(self.console.clone()) diff --git a/tests/fixtures/python/base/.gitignore b/tests/fixtures/python/base/.gitignore new file mode 100644 index 00000000000..1d17dae13b5 --- /dev/null +++ b/tests/fixtures/python/base/.gitignore @@ -0,0 +1 @@ +.venv diff --git a/website/docs/config/toolchain.mdx b/website/docs/config/toolchain.mdx index f8d0a0e410e..ce75b21f964 100644 --- a/website/docs/config/toolchain.mdx +++ b/website/docs/config/toolchain.mdx @@ -758,19 +758,18 @@ Python installation's are based on pre-built binaries provided by Supports the "single version policy" or "one version rule" patterns by only allowing dependencies in the root `requirements.txt`, and only installing dependencies in the workspace root, and not within individual projects. It also bypasses all `workspaces` checks to determine package locations. -Defaults to `true`. ```yaml title=".moon/toolchain.yml" {2} python: - rootRequirementsOnly: false + rootRequirementsOnly: true ``` ### `venvName` -Defines the virtual environment file name, which will be created in the workspace or project root, -and where dependencies will be installed into. Defaults to `.venv` +Defines the virtual environment name, which will be created in the workspace or project root when a +`requirements.txt` exists, and where dependencies will be installed into. Defaults to `.venv` ```yaml title=".moon/toolchain.yml" {2} python: