Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: start adding interface to override #834

Merged
merged 36 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
726c633
start adding interface to override
SobhanMP Aug 22, 2024
02d1fc8
complete the interface
SobhanMP Aug 22, 2024
84b362b
handle empty strings again
SobhanMP Aug 23, 2024
aa8e7e1
fix test
SobhanMP Aug 23, 2024
6732316
iter
SobhanMP Aug 27, 2024
16beb97
remove unused import
SobhanMP Aug 27, 2024
c32b28c
Merge branch 'main' into env
SobhanMP Aug 27, 2024
de3f331
use as_deref
SobhanMP Aug 27, 2024
2a9ffdd
add quad state overrides
SobhanMP Aug 28, 2024
8fad46f
fmt
SobhanMP Aug 28, 2024
8bbd546
remove useless methods
SobhanMP Aug 28, 2024
28e07ac
make python binding
SobhanMP Aug 28, 2024
1e030e9
add a few more tests, do not use default env var name, just in case
SobhanMP Aug 28, 2024
06dfb06
add a single python unit test
SobhanMP Aug 28, 2024
2dc4577
add a python test for good measure
SobhanMP Aug 28, 2024
3810861
add annotation
SobhanMP Aug 28, 2024
def7eb6
fix doc
SobhanMP Aug 28, 2024
f4a9803
annotate test
SobhanMP Aug 28, 2024
184f1a4
fmt
SobhanMP Aug 28, 2024
15a9b4b
Update crates/rattler_virtual_packages/src/lib.rs
SobhanMP Aug 29, 2024
237c669
Update py-rattler/rattler/virtual_package/virtual_package.py
SobhanMP Aug 29, 2024
a7cabed
rename some stuff
SobhanMP Aug 29, 2024
d83dfb7
Merge branch 'env' of github.com:SobhanMP/rattler into env
SobhanMP Aug 29, 2024
8f17402
fmt, stop using current
SobhanMP Aug 29, 2024
1618ae0
bump
SobhanMP Aug 29, 2024
a6a8403
mypy
SobhanMP Aug 29, 2024
752bdd2
ruf
SobhanMP Aug 29, 2024
976c30f
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
SobhanMP Aug 29, 2024
fb21bf4
Update crates/rattler_virtual_packages/src/lib.rs
SobhanMP Aug 29, 2024
b79dde0
apply @Baszalmastra's suggetions
SobhanMP Aug 29, 2024
d6c8d59
use `Default::default()`
SobhanMP Aug 29, 2024
157c813
fmt
SobhanMP Aug 29, 2024
1c03db5
fix test
SobhanMP Aug 29, 2024
9057932
change deprecation
SobhanMP Aug 29, 2024
5aa6af8
oops
SobhanMP Aug 29, 2024
24b24f8
Merge branch 'main' into env
SobhanMP Aug 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 138 additions & 19 deletions crates/rattler_virtual_packages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,56 @@ pub trait EnvOverride: Sized {
/// - `None`, then the environment variable did not exist,
/// - `Some(Err(None))`, then the environment variable exist but was set to zero, so the package should be disabled
/// - `Some(Ok(pkg))`, then the override was for the package.
fn from_env_var_name(env_var_name: &str) -> Option<Result<Self, Option<ParseVersionError>>> {
let var = env::var(env_var_name).ok()?;
if var.is_empty() {
Some(Err(None))
} else {
Some(Self::from_env_var_name_with_var(env_var_name, &var).map_err(Some))
fn from_env_var_name_or<F>(
env_var_name: &str,
f: F,
SobhanMP marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Option<Self>, DetectVirtualPackageError>
where
F: FnOnce() -> Result<Option<Self>, DetectVirtualPackageError>,
{
match env::var(env_var_name) {
Ok(var) => {
if var.is_empty() {
Ok(None)
} else {
Ok(Some(Self::from_env_var_name_with_var(env_var_name, &var)?))
}
}
Err(env::VarError::NotPresent) => f(),
Err(e) => Err(DetectVirtualPackageError::VarError(e)),
}
}

/// Helper method for [`EnvOverride::from_env_var_name_or`] that uses [`EnvOverride::current`] as the default.
fn from_env_var_name_or_current(
env_var_name: &str,
) -> Result<Option<Self>, DetectVirtualPackageError> {
Self::from_env_var_name_or(env_var_name, Self::current)
}

/// Helper method for [`EnvOverride::from_env_var_name_or`] that does not have any default.
fn from_env_var_name(env_var_name: &str) -> Result<Option<Self>, DetectVirtualPackageError> {
Self::from_env_var_name_or(env_var_name, || Ok(None))
}

/// Default name of the environment variable that overrides the virtual package.
const DEFAULT_ENV_NAME: &'static str;

/// Shortcut for `EnvOverride::from_env_var_name(EnvOverride::DEFAULT_ENV_NAME)`.
fn from_default_env_var() -> Option<Result<Self, Option<ParseVersionError>>> {
Self::from_env_var_name(Self::DEFAULT_ENV_NAME)
fn from_default_env_var_or<F>(f: F) -> Result<Option<Self>, DetectVirtualPackageError>
where
F: FnOnce() -> Result<Option<Self>, DetectVirtualPackageError>,
{
Self::from_env_var_name_or(Self::DEFAULT_ENV_NAME, f)
}

/// Shortcut for `EnvOverride::from_env_var_name(EnvOverride::DEFAULT_ENV_NAME)`.
fn from_default_env_var() -> Result<Option<Self>, DetectVirtualPackageError> {
Self::from_default_env_var_or(|| Ok(None))
}

/// This method is here so that `<Self as EnvOverride>::current` always returns the same error type.
fn current() -> Result<Option<Self>, DetectVirtualPackageError>;
}

/// An enum that represents all virtual package types provided by this library.
Expand Down Expand Up @@ -136,6 +170,24 @@ impl VirtualPackage {
.get_or_try_init(try_detect_virtual_packages)
.map(Vec::as_slice)
}

/// disable overrides
pub fn current_no_overrides() -> Result<&'static [Self], DetectVirtualPackageError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isnt this just the same as the current method?

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 changed the default to match conda (i.e. use the default overrides)

static DETECTED_VIRTUAL_PACKAGES: OnceCell<Vec<VirtualPackage>> = OnceCell::new();
DETECTED_VIRTUAL_PACKAGES
.get_or_try_init(try_detect_virtual_packages_no_overrides)
.map(Vec::as_slice)
}

/// use custom overrides
pub fn current_with_overrides(
overrides: &VirtualPackageOverride<'_>,
) -> Result<&'static [Self], DetectVirtualPackageError> {
static DETECTED_VIRTUAL_PACKAGES: OnceCell<Vec<VirtualPackage>> = OnceCell::new();
DETECTED_VIRTUAL_PACKAGES
Copy link
Collaborator

Choose a reason for hiding this comment

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

This caches the values but based on the input which means if you call it a second time with different input you get the same result as rhe first time. I think it would be better to not cache the values here and return a Vec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

.get_or_try_init(|| try_detect_virtual_packages_with_overrides(overrides))
.map(Vec::as_slice)
}
}

/// An error that might be returned by [`VirtualPackage::current`].
Expand All @@ -150,10 +202,46 @@ pub enum DetectVirtualPackageError {

#[error(transparent)]
DetectLibC(#[from] DetectLibCError),

#[error(transparent)]
VarError(#[from] env::VarError),

#[error(transparent)]
VersionParseError(#[from] ParseVersionError),
}

/// Configure the overrides used in in this crate.
SobhanMP marked this conversation as resolved.
Show resolved Hide resolved

pub struct VirtualPackageOverride<'a> {
osx: Option<&'a str>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use an Option<String> to remove the need for lifetimes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if maybe we should change this to a trie state:

  1. dont use any override
  2. use the default env var if it exists,
  3. use the environment variable with a specific name.

that way we can get rid of all the slightly different functions.

WDYT?

Copy link
Contributor Author

@SobhanMP SobhanMP Aug 27, 2024

Choose a reason for hiding this comment

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

I agree, but how about a 4th option that forces a specific version? (I think @iamthebot would like that), I'll give this a try, and if it doesn't work, we can revert to the last commit.

libc: Option<&'a str>,
cuda: Option<&'a str>,
}

impl VirtualPackageOverride<'static> {
/// Disable all overrides
pub fn none() -> Self {
Self {
osx: None,
libc: None,
cuda: None,
}
}

/// Use the default overrides of Conda.
fn default() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please implement Default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default as in the default trait? Will do, did not know it was a thing.

Self {
osx: Some(Osx::DEFAULT_ENV_NAME),
libc: Some(LibC::DEFAULT_ENV_NAME),
cuda: Some(Cuda::DEFAULT_ENV_NAME),
}
}
}

// Detect the available virtual packages on the system
fn try_detect_virtual_packages() -> Result<Vec<VirtualPackage>, DetectVirtualPackageError> {
fn try_detect_virtual_packages_with_overrides(
overrides: &VirtualPackageOverride<'_>,
) -> Result<Vec<VirtualPackage>, DetectVirtualPackageError> {
let mut result = Vec::new();
let platform = Platform::current();

Expand All @@ -169,18 +257,27 @@ fn try_detect_virtual_packages() -> Result<Vec<VirtualPackage>, DetectVirtualPac
if let Some(linux_version) = Linux::current()? {
result.push(linux_version.into());
}
if let Some(libc) = LibC::current()? {
if let Some(libc) = overrides.libc.map_or_else(
<LibC as EnvOverride>::current,
LibC::from_env_var_name_or_current,
)? {
result.push(libc.into());
}
}

if platform.is_osx() {
if let Some(osx) = Osx::current()? {
if let Some(osx) = overrides.osx.map_or_else(
<Osx as EnvOverride>::current,
Osx::from_env_var_name_or_current,
)? {
result.push(osx.into());
}
}

if let Some(cuda) = Cuda::current() {
if let Some(cuda) = overrides.cuda.map_or_else(
<Cuda as EnvOverride>::current,
Cuda::from_env_var_name_or_current,
)? {
result.push(cuda.into());
}

Expand All @@ -191,6 +288,15 @@ fn try_detect_virtual_packages() -> Result<Vec<VirtualPackage>, DetectVirtualPac
Ok(result)
}

fn try_detect_virtual_packages() -> Result<Vec<VirtualPackage>, DetectVirtualPackageError> {
try_detect_virtual_packages_with_overrides(&VirtualPackageOverride::default())
}

fn try_detect_virtual_packages_no_overrides(
) -> Result<Vec<VirtualPackage>, DetectVirtualPackageError> {
try_detect_virtual_packages_with_overrides(&VirtualPackageOverride::none())
}

/// Linux virtual package description
#[derive(Clone, Eq, PartialEq, Hash, Debug, Deserialize)]
pub struct Linux {
Expand Down Expand Up @@ -283,6 +389,10 @@ impl EnvOverride for LibC {
version,
})
}

fn current() -> Result<Option<Self>, DetectVirtualPackageError> {
Ok(Self::current()?)
}
}

/// Cuda virtual package description
Expand Down Expand Up @@ -312,7 +422,9 @@ impl EnvOverride for Cuda {
) -> Result<Self, ParseVersionError> {
Version::from_str(env_var_value).map(|version| Self { version })
}

fn current() -> Result<Option<Self>, DetectVirtualPackageError> {
Ok(Self::current())
}
const DEFAULT_ENV_NAME: &'static str = "CONDA_OVERRIDE_CUDA";
}

Expand Down Expand Up @@ -489,7 +601,9 @@ impl EnvOverride for Osx {
) -> Result<Self, ParseVersionError> {
Version::from_str(env_var_value).map(|version| Self { version })
}

fn current() -> Result<Option<Self>, DetectVirtualPackageError> {
Ok(Self::current()?)
}
const DEFAULT_ENV_NAME: &'static str = "CONDA_OVERRIDE_OSX";
}

Expand Down Expand Up @@ -519,11 +633,16 @@ mod test {
family: "glibc".into(),
};
env::set_var(LibC::DEFAULT_ENV_NAME, v);
assert_eq!(LibC::from_default_env_var(), Some(Ok(res)));
assert_eq!(LibC::from_default_env_var().unwrap().unwrap(), res);
env::set_var(LibC::DEFAULT_ENV_NAME, "");
assert_eq!(LibC::from_default_env_var(), Some(Err(None)));
assert_eq!(LibC::from_default_env_var().unwrap(), None);
env::remove_var(LibC::DEFAULT_ENV_NAME);
assert_eq!(LibC::from_default_env_var(), None);
assert_eq!(
LibC::from_default_env_var_or(|| Ok(Some(res.clone())))
.unwrap()
.unwrap(),
res
);
}

#[test]
Expand All @@ -533,7 +652,7 @@ mod test {
version: Version::from_str(v).unwrap(),
};
env::set_var(Cuda::DEFAULT_ENV_NAME, v);
assert_eq!(Cuda::from_default_env_var(), Some(Ok(res)));
assert_eq!(Cuda::from_default_env_var().unwrap().unwrap(), res);
}

#[test]
Expand All @@ -543,6 +662,6 @@ mod test {
version: Version::from_str(v).unwrap(),
};
env::set_var(Osx::DEFAULT_ENV_NAME, v);
assert_eq!(Osx::from_default_env_var(), Some(Ok(res)));
assert_eq!(Osx::from_default_env_var().unwrap().unwrap(), res);
}
}
2 changes: 1 addition & 1 deletion crates/rattler_virtual_packages/src/libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub enum DetectLibCError {
#[cfg(unix)]
fn try_detect_libc_version() -> Result<Option<(String, Version)>, DetectLibCError> {
// Run `ldd --version` to detect the libc version and family on the system.
// `ldd` is shipped with libc so if an error occured during its execution we
// `ldd` is shipped with libc so if an error occurred during its execution we
// can assume no libc is available on the system.
let output = match std::process::Command::new("ldd").arg("--version").output() {
Err(e) => {
Expand Down