Skip to content

Commit

Permalink
fix: invert pyc filter to check against old files and include all new…
Browse files Browse the repository at this point in the history
… `.pyc` files (#1246)
  • Loading branch information
wolfv authored Dec 9, 2024
1 parent 1d28667 commit 96c449f
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 16 deletions.
6 changes: 5 additions & 1 deletion src/packaging/file_finder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ use super::{file_mapper, PackagingError};
pub struct Files {
/// The files that are new in the prefix
pub new_files: HashSet<PathBuf>,
/// The files that were present in the original conda environment
pub old_files: HashSet<PathBuf>,
/// The prefix that we are dealing with
pub prefix: PathBuf,
}
Expand Down Expand Up @@ -74,6 +76,7 @@ impl Files {
if !prefix.exists() {
return Ok(Files {
new_files: HashSet::new(),
old_files: HashSet::new(),
prefix: prefix.to_owned(),
});
}
Expand Down Expand Up @@ -119,6 +122,7 @@ impl Files {

Ok(Files {
new_files: difference,
old_files: previous_files,
prefix: prefix.to_owned(),
})
}
Expand All @@ -130,7 +134,7 @@ impl Files {
let mut content_type_map = HashMap::new();
for f in &self.new_files {
// temporary measure to remove pyc files that are not supposed to be there
if file_mapper::filter_pyc(f, &self.new_files) {
if file_mapper::filter_pyc(f, &self.old_files) {
continue;
}

Expand Down
49 changes: 45 additions & 4 deletions src/packaging/file_mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ use std::{

use super::PackagingError;

/// We check that each `pyc` file in the package is also present as a `py` file.
/// We check the (new) `pyc` files against the old files from the environment.
/// This is a temporary measure to avoid packaging `pyc` files that are not
/// generated by the build process.
pub fn filter_pyc(path: &Path, new_files: &HashSet<PathBuf>) -> bool {
pub fn filter_pyc(path: &Path, old_files: &HashSet<PathBuf>) -> bool {
if let (Some(ext), Some(parent)) = (path.extension(), path.parent()) {
if ext == "pyc" {
let has_pycache = parent.ends_with("__pycache__");
Expand All @@ -32,13 +32,13 @@ pub fn filter_pyc(path: &Path, new_files: &HashSet<PathBuf>) -> bool {
if let Some(pp) = parent.parent() {
pp.join(format!("{}.py", py_stem))
} else {
return true;
return false;
}
} else {
path.with_extension("py")
};

if !new_files.contains(&pyfile) {
if old_files.contains(&pyfile) {
return true;
}
}
Expand Down Expand Up @@ -292,6 +292,13 @@ impl Output {

#[cfg(test)]
mod test {
use std::{
collections::HashSet,
path::{Path, PathBuf},
};

use crate::packaging::file_mapper::filter_pyc;

#[test]
fn test_filter_file() {
let test_cases = vec![
Expand All @@ -317,4 +324,38 @@ mod test {
assert_eq!(super::filter_file(path), expected);
}
}

#[test]
fn test_filter_pyc() {
let mut old_files = HashSet::new();
old_files.insert(PathBuf::from("pkg/module.py"));
old_files.insert(PathBuf::from("pkg/other.py"));
old_files.insert(PathBuf::from("pkg/nested/deep.py"));

let test_cases = vec![
// __pycache__ cases
("pkg/__pycache__/module.cpython-311.pyc", true), // has corresponding .py
("pkg/__pycache__/missing.cpython-311.pyc", false), // no corresponding .py
// Regular pyc files
("pkg/module.pyc", true), // has corresponding .py
("pkg/missing.pyc", false), // no corresponding .py
// Nested paths
("pkg/nested/__pycache__/deep.cpython-311.pyc", true),
("pkg/nested/deep.pyc", true),
// Edge cases
("pkg/not_python.txt", false), // non-python files pass through
("pkg/__pycache__/invalid", false), // no extension
("", false), // empty path
];

for (file, expected) in test_cases {
let path = Path::new(file);
assert_eq!(
filter_pyc(path, &old_files),
expected,
"Failed for path: {}",
file
);
}
}
}
3 changes: 3 additions & 0 deletions test-data/recipes/python_compilation/recipe.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,6 @@

py_broken = prefix / "broken.py"
py_broken.write_text("print('Hello, world!'")

adding_pyc_file = prefix / "just_a_.cpython-311.pyc"
adding_pyc_file.write_text("")
8 changes: 2 additions & 6 deletions test-data/recipes/python_compilation/recipe.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@ package:
build:
number: 0
script:
- if: unix
then:
- python $RECIPE_DIR/recipe.py
- if: win
then:
- python %RECIPE_DIR%\recipe.py
interpreter: python
file: recipe.py

python:
skip_pyc_compilation:
Expand Down
17 changes: 12 additions & 5 deletions test/end-to-end/test_simple.py
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,9 @@ def test_compile_python(rattler_build: RattlerBuild, recipes: Path, tmp_path: Pa

assert (pkg / "info/paths.json").exists()
paths = json.loads((pkg / "info/paths.json").read_text())
assert (
len([p for p in paths["paths"] if p["_path"].endswith(".cpython-311.pyc")]) == 2
)
pyc_paths = [p["_path"] for p in paths["paths"] if p["_path"].endswith(".pyc")]
assert len(pyc_paths) == 3
assert "just_a_.cpython-311.pyc" in pyc_paths
assert len([p for p in paths["paths"] if p["_path"].endswith(".py")]) == 4

# make sure that we include the `info/recipe/recipe.py` file
Expand Down Expand Up @@ -1102,11 +1102,18 @@ def test_pin_compatible(

assert snapshot_json == rendered[0]["recipe"]["requirements"]


def test_render_variants(
rattler_build: RattlerBuild, recipes: Path, tmp_path: Path, snapshot_json
):
rendered = rattler_build.render(recipes / "race-condition/recipe-undefined-variant.yaml", tmp_path)
assert [rx["recipe"]["package"]["name"] for rx in rendered] == ["my-package-a", "my-package-b"]
rendered = rattler_build.render(
recipes / "race-condition/recipe-undefined-variant.yaml", tmp_path
)
assert [rx["recipe"]["package"]["name"] for rx in rendered] == [
"my-package-a",
"my-package-b",
]


def test_race_condition(
rattler_build: RattlerBuild, recipes: Path, tmp_path: Path, snapshot_json
Expand Down

0 comments on commit 96c449f

Please sign in to comment.