Skip to content

Commit

Permalink
fix either feature conditional compilation, again (#3834)
Browse files Browse the repository at this point in the history
* fix `either` feature conditional compilation, again

* test feature powerset in CI

* install `rust-src` for feature powerset tests

* review: adamreichold feedback

* Fix one more case of redundant imports.

* just check feature powerset for now

---------

Co-authored-by: Adam Reichold <adam.reichold@t-online.de>
  • Loading branch information
davidhewitt and adamreichold committed Feb 22, 2024
1 parent c375c8a commit 482883a
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 15 deletions.
17 changes: 17 additions & 0 deletions .github/workflows/ci.yml
Expand Up @@ -459,6 +459,22 @@ jobs:
- run: python3 -m pip install --upgrade pip && pip install nox
- run: python3 -m nox -s test-version-limits

check-feature-powerset:
needs: [fmt]
if: ${{ contains(github.event.pull_request.labels.*.name, 'CI-build-full') || (github.event_name != 'pull_request' && github.ref != 'refs/heads/main') }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
- uses: Swatinem/rust-cache@v2
continue-on-error: true
- uses: dtolnay/rust-toolchain@stable
with:
components: rust-src
- uses: taiki-e/install-action@cargo-hack
- run: python3 -m pip install --upgrade pip && pip install nox
- run: python3 -m nox -s check-feature-powerset

conclusion:
needs:
- fmt
Expand All @@ -473,6 +489,7 @@ jobs:
- emscripten
- test-debug
- test-version-limits
- check-feature-powerset
if: always()
runs-on: ubuntu-latest
steps:
Expand Down
16 changes: 8 additions & 8 deletions Cargo.toml
Expand Up @@ -103,18 +103,18 @@ nightly = []
full = [
"macros",
# "multiple-pymethods", # TODO re-add this when MSRV is greater than 1.62
"anyhow",
"chrono",
"num-bigint",
"num-complex",
"hashbrown",
"smallvec",
"serde",
"indexmap",
"either",
"eyre",
"anyhow",
"experimental-inspect",
"eyre",
"hashbrown",
"indexmap",
"num-bigint",
"num-complex",
"rust_decimal",
"serde",
"smallvec",
]

[workspace]
Expand Down
1 change: 1 addition & 0 deletions newsfragments/3834.fixed.md
@@ -0,0 +1 @@
Fix compilation failure with `either` feature enabled without `experimental-inspect` enabled.
82 changes: 78 additions & 4 deletions noxfile.py
Expand Up @@ -13,6 +13,14 @@
import nox
import nox.command

try:
import tomllib as toml
except ImportError:
try:
import toml
except ImportError:
toml = None

nox.options.sessions = ["test", "clippy", "rustfmt", "ruff", "docs"]


Expand Down Expand Up @@ -467,10 +475,8 @@ def check_changelog(session: nox.Session):
def set_minimal_package_versions(session: nox.Session):
from collections import defaultdict

try:
import tomllib as toml
except ImportError:
import toml
if toml is None:
session.error("requires Python 3.11 or `toml` to be installed")

projects = (
None,
Expand Down Expand Up @@ -581,6 +587,74 @@ def test_version_limits(session: nox.Session):
_run_cargo(session, "check", env=env, expect_error=True)


@nox.session(name="check-feature-powerset", venv_backend="none")
def check_feature_powerset(session: nox.Session):
if toml is None:
session.error("requires Python 3.11 or `toml` to be installed")

with (PYO3_DIR / "Cargo.toml").open("rb") as cargo_toml_file:
cargo_toml = toml.load(cargo_toml_file)

EXCLUDED_FROM_FULL = {
"nightly",
"extension-module",
"full",
"default",
"auto-initialize",
"generate-import-lib",
"multiple-pymethods", # TODO add this after MSRV 1.62
}

features = cargo_toml["features"]

full_feature = set(features["full"])
abi3_features = {feature for feature in features if feature.startswith("abi3")}
abi3_version_features = abi3_features - {"abi3"}

expected_full_feature = features.keys() - EXCLUDED_FROM_FULL - abi3_features

uncovered_features = expected_full_feature - full_feature
if uncovered_features:
session.error(
f"some features missing from `full` meta feature: {uncovered_features}"
)

experimental_features = {
feature for feature in features if feature.startswith("experimental-")
}
full_without_experimental = full_feature - experimental_features

if len(experimental_features) >= 2:
# justification: we always assume that feature within these groups are
# mutually exclusive to simplify CI
features_to_group = [
full_without_experimental,
experimental_features,
]
elif len(experimental_features) == 1:
# no need to make an experimental features group
features_to_group = [full_without_experimental]
else:
session.error("no experimental features exist; please simplify the noxfile")

features_to_skip = [
*EXCLUDED_FROM_FULL,
*abi3_version_features,
]

comma_join = ",".join
_run_cargo(
session,
"hack",
"--feature-powerset",
'--optional-deps=""',
f'--skip="{comma_join(features_to_skip)}"',
*(f"--group-features={comma_join(group)}" for group in features_to_group),
"check",
"--all-targets",
)


def _build_docs_for_ffi_check(session: nox.Session) -> None:
# pyo3-ffi-check needs to scrape docs of pyo3-ffi
_run_cargo(session, "doc", _FFI_CHECK, "-p", "pyo3-ffi", "--no-deps")
Expand Down
1 change: 0 additions & 1 deletion pytests/src/comparisons.rs
@@ -1,5 +1,4 @@
use pyo3::prelude::*;
use pyo3::{types::PyModule, Python};

#[pyclass]
struct Eq(i64);
Expand Down
10 changes: 8 additions & 2 deletions src/conversions/either.rs
Expand Up @@ -93,7 +93,13 @@ where
} else if let Ok(r) = obj.extract::<R>() {
Ok(Either::Right(r))
} else {
let err_msg = format!("failed to convert the value to '{}'", Self::type_input());
// TODO: it might be nice to use the `type_input()` name here once `type_input`
// is not experimental, rather than the Rust type names.
let err_msg = format!(
"failed to convert the value to 'Union[{}, {}]'",
std::any::type_name::<L>(),
std::any::type_name::<R>()
);
Err(PyTypeError::new_err(err_msg))
}
}
Expand Down Expand Up @@ -133,7 +139,7 @@ mod tests {
assert!(err.is_instance_of::<PyTypeError>(py));
assert_eq!(
err.to_string(),
"TypeError: failed to convert the value to 'Union[int, float]'"
"TypeError: failed to convert the value to 'Union[i32, f32]'"
);

let obj_i = 42.to_object(py);
Expand Down

0 comments on commit 482883a

Please sign in to comment.