Skip to content

Commit

Permalink
fix: trampoline without corresponding json breaking (prefix-dev#2576)
Browse files Browse the repository at this point in the history 
Fixes prefix-dev#2537

- Fix behaviour
- Add test
- Rename `GlobalBin` to `GlobalExecutable`
- Add logging messages
- Remove `test_sync_clean_up_broken_exec` -> we only clean up invalid
trampoline-configuration pairs now
  • Loading branch information
@Hofer-Julian @jjjermiah
Hofer-Julian authored and jjjermiah committed Nov 30, 2024
1 parent 89e6b36 commit ad06992
Show file tree
Hide file tree
Showing 6 changed files with 162 additions and 130 deletions.
20 changes: 10 additions & 10 deletions src/global/common.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::trampoline::{GlobalBin, Trampoline};
use super::trampoline::{GlobalExecutable, Trampoline};
use super::{EnvironmentName, ExposedName, Mapping};
use crate::prefix::Executable;

Expand All @@ -22,6 +22,7 @@ use rattler_conda_types::{
use std::collections::HashMap;
use std::ffi::OsStr;
use std::iter::Peekable;
use std::ops::Not;
use std::str::FromStr;
use std::{
io::Read,
Expand Down Expand Up @@ -57,19 +58,18 @@ impl BinDir {
///
/// This function reads the directory specified by `self.0` and try to collect all
/// file paths into a vector. It returns a `miette::Result` containing the
/// vector of `GlobalBin`or an error if the directory can't be read.
pub(crate) async fn bins(&self) -> miette::Result<Vec<GlobalBin>> {
/// vector of `GlobalExecutable`or an error if the directory can't be read.
pub(crate) async fn executables(&self) -> miette::Result<Vec<GlobalExecutable>> {
let mut files = Vec::new();
let mut entries = tokio_fs::read_dir(&self.0).await.into_diagnostic()?;

while let Some(entry) = entries.next_entry().await.into_diagnostic()? {
let path = entry.path();
if path.is_file() && path.is_executable() && Trampoline::is_trampoline(&path).await? {
let trampoline = Trampoline::try_from(path).await?;
files.push(GlobalBin::Trampoline(trampoline));
} else if path.is_file() && path.is_executable() && !is_binary(&path)? {
if let Ok(trampoline) = Trampoline::try_from(&path).await {
files.push(GlobalExecutable::Trampoline(trampoline));
} else if path.is_file() && path.is_executable() && is_binary(&path)?.not() {
// If the file is not a binary, it's a script
files.push(GlobalBin::Script(path));
files.push(GlobalExecutable::Script(path));
}
}

Expand Down Expand Up @@ -704,9 +704,9 @@ pub(crate) async fn get_expose_scripts_sync_status(
bin_dir: &BinDir,
env_dir: &EnvDir,
mappings: &IndexSet<Mapping>,
) -> miette::Result<(Vec<GlobalBin>, IndexSet<ExposedName>)> {
) -> miette::Result<(Vec<GlobalExecutable>, IndexSet<ExposedName>)> {
// Get all paths to the binaries from trampolines or scripts in the bin directory.
let locally_exposed = bin_dir.bins().await?;
let locally_exposed = bin_dir.executables().await?;
let executable_paths = futures::future::join_all(locally_exposed.iter().map(|global_bin| {
let global_bin = global_bin.clone();
let path = global_bin.path().clone();
Expand Down
18 changes: 13 additions & 5 deletions src/global/install.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ pub(crate) async fn create_executable_trampolines(
original_executable,
} in mapped_executables
{
tracing::debug!("Create trampoline {}", global_script_path.display());
let exe = prefix.root().join(original_executable);
let path = prefix
.root()
Expand All @@ -118,7 +119,14 @@ pub(crate) async fn create_executable_trampolines(
})?);
let metadata = Configuration::new(exe, path, Some(activation_variables.clone()));

let json_path = Configuration::trampoline_configuration(global_script_path);
let parent_dir = global_script_path.parent().ok_or_else(|| {
miette::miette!(
"{} needs to have a parent directory",
global_script_path.display()
)
})?;
let exposed_name = Trampoline::name(global_script_path)?;
let json_path = Configuration::path_from_trampoline(parent_dir, &exposed_name);

// Check if an old bash script is present and remove it
let mut changed = if global_script_path.exists()
Expand Down Expand Up @@ -463,7 +471,7 @@ mod tests {
#[cfg(windows)]
#[tokio::test]
async fn test_extract_executable_from_script_windows() {
use crate::global::trampoline::GlobalBin;
use crate::global::trampoline::GlobalExecutable;
use std::fs;
use std::path::Path;
let script_without_quote = r#"
Expand All @@ -475,7 +483,7 @@ mod tests {
let tempdir = tempfile::tempdir().unwrap();
let script_path = tempdir.path().join(script_path);
fs::write(&script_path, script_without_quote).unwrap();
let script_global_bin = GlobalBin::Script(script_path);
let script_global_bin = GlobalExecutable::Script(script_path);
let executable_path = script_global_bin.executable().await.unwrap();
assert_eq!(
executable_path,
Expand All @@ -502,7 +510,7 @@ mod tests {
async fn test_extract_executable_from_script_unix() {
use std::{fs, path::Path};

use crate::global::trampoline::GlobalBin;
use crate::global::trampoline::GlobalExecutable;

let script = r#"#!/bin/sh
export PATH="/home/user/.pixi/envs/nushell/bin:${PATH}"
Expand All @@ -513,7 +521,7 @@ export CONDA_PREFIX="/home/user/.pixi/envs/nushell"
let tempdir = tempfile::tempdir().unwrap();
let script_path = tempdir.path().join(script_path);
fs::write(&script_path, script).unwrap();
let script_global_bin = GlobalBin::Script(script_path);
let script_global_bin = GlobalExecutable::Script(script_path);
let executable_path = script_global_bin.executable().await.unwrap();
assert_eq!(
executable_path,
Expand Down
87 changes: 51 additions & 36 deletions src/global/project/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use self::trampoline::{Configuration, ConfigurationParseError, Trampoline};

use super::common::{get_install_changes, EnvironmentUpdate};
use super::install::find_binary_by_name;
use super::trampoline::GlobalBin;
use super::trampoline::{self, GlobalExecutable};
use super::{BinDir, EnvRoot, StateChange, StateChanges};
use crate::global::common::{
channel_url_to_prioritized_channel, find_package_records, get_expose_scripts_sync_status,
Expand All @@ -22,6 +24,7 @@ use fs::tokio as tokio_fs;
use fs_err as fs;
use futures::stream::StreamExt;
use indexmap::{IndexMap, IndexSet};
use is_executable::IsExecutable;
use itertools::Itertools;
pub(crate) use manifest::{ExposedType, Manifest, Mapping};
use miette::{miette, Context, IntoDiagnostic};
Expand Down Expand Up @@ -113,7 +116,7 @@ impl ExposedData {
/// reading the associated `conda-meta` directory.
/// or it looks into the trampoline manifest to extract the metadata.
pub async fn from_exposed_path(
bin: &GlobalBin,
bin: &GlobalExecutable,
env_root: &EnvRoot,
channel_config: &ChannelConfig,
) -> miette::Result<Self> {
Expand Down Expand Up @@ -286,17 +289,25 @@ impl Project {
let env_root = EnvRoot::from_env().await?;

if !manifest_path.exists() {
tracing::debug!(
"Global manifest {} doesn't exist yet. Creating a new one.",
manifest_path.display()
);
tokio_fs::create_dir_all(&manifest_dir)
.await
.into_diagnostic()?;

if !env_root.directories().await?.is_empty() {
tracing::debug!(
"Existing installation found. Creating global manifest from that information."
);
return Self::try_from_existing_installation(&manifest_path, env_root, bin_dir)
.await
.wrap_err_with(|| {
"Failed to create global manifest from existing installation"
});
} else {
tracing::debug!("Create an empty global manifest.");
tokio_fs::File::create(&manifest_path)
.await
.into_diagnostic()?;
Expand All @@ -314,7 +325,7 @@ impl Project {
let config = Config::load(env_root.path());

let exposed_binaries: Vec<ExposedData> = bin_dir
.bins()
.executables()
.await?
.into_iter()
.map(|bin| {
Expand Down Expand Up @@ -793,7 +804,7 @@ impl Project {
return Ok(false);
}

// Verify the binaries to be in sync with the environment
tracing::debug!("Verify that the binaries are in sync with the environment");
let (to_remove, to_add) =
get_expose_scripts_sync_status(&self.bin_dir, &env_dir, &environment.exposed).await?;
if !to_remove.is_empty() || !to_add.is_empty() {
Expand Down Expand Up @@ -896,9 +907,9 @@ impl Project {
// Prune environments that are not listed
state_changes |= self.prune_old_environments().await?;

// Remove broken scripts
if let Err(err) = self.remove_broken_bins().await {
tracing::warn!("Couldn't remove broken exposed executables: {err}")
// Remove broken files
if let Err(err) = self.remove_broken_files().await {
tracing::warn!("Couldn't remove broken files: {err}")
}

for env_name in self.environments().keys() {
Expand All @@ -916,9 +927,14 @@ impl Project {
removed_packages: Option<Vec<PackageName>>,
) -> miette::Result<StateChanges> {
let mut state_changes = StateChanges::new_with_env(env_name.clone());
if !self.environment_in_sync(env_name).await? {
if self.environment_in_sync(env_name).await? {
tracing::debug!(
"Environment {} specs already up to date with global manifest",
env_name.fancy_display()
);
} else {
tracing::debug!(
"Environment {} specs not up to date with manifest",
"Environment {} specs not up to date with global manifest",
env_name.fancy_display()
);
let mut environment_update = self.install_environment(env_name).await?;
Expand All @@ -940,37 +956,36 @@ impl Project {
}

/// Delete scripts in the bin folder that are broken
pub(crate) async fn remove_broken_bins(&self) -> miette::Result<()> {
for exposed_bin in self.bin_dir.bins().await? {
let executable = exposed_bin.executable().await;

if executable
.and_then(|path| {
if path.is_file() {
Ok(path)
} else {
miette::bail!("Path is not a file")
pub(crate) async fn remove_broken_files(&self) -> miette::Result<()> {
// Get all the files in the global binary directory
// If there's a trampoline that couldn't be correctly parsed, remove it
let root_path = self.bin_dir.path();
let mut entries = tokio_fs::read_dir(&root_path).await.into_diagnostic()?;

while let Some(entry) = entries.next_entry().await.into_diagnostic()? {
let path = entry.path();
if path.is_file() && path.is_executable() && Trampoline::is_trampoline(&path).await? {
let exposed_name = Trampoline::name(&path)?;
match Configuration::from_root_path(root_path, &exposed_name).await {
Ok(_) => (),
Err(ConfigurationParseError::ReadError(config_path, err)) => {
tracing::warn!("Couldn't read {}: {err}", config_path.display());
tracing::warn!("Removing the trampoline at {}", path.display());
tokio_fs::remove_file(path).await.into_diagnostic()?;
}
Err(ConfigurationParseError::ParseError(config_path, err)) => {
tracing::warn!("Couldn't parse {}: {err}", config_path.display());
tracing::warn!(
"Removing the trampoline at {} and configuration at {}",
path.display(),
config_path.display()
);
tokio_fs::remove_file(path).await.into_diagnostic()?;
tokio_fs::remove_file(config_path).await.into_diagnostic()?;
}
})
.is_err()
{
tokio_fs::remove_file(exposed_bin.path())
.await
.into_diagnostic()?;

if exposed_bin.is_trampoline() {
tokio_fs::remove_file(
exposed_bin
.trampoline()
.expect("we checked it")
.manifest_path(),
)
.await
.into_diagnostic()?;
}
}
}

Ok(())
}

Expand Down
Loading

0 comments on commit ad06992

Please sign in to comment.