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

Add the conda package manager #296461

Merged
merged 7 commits into from
Apr 26, 2024
Merged

Conversation

EricTheMagician
Copy link
Contributor

@EricTheMagician EricTheMagician commented Mar 16, 2024

Description of changes

I wanted to add a newer version of the conda package manager without breaking the existing conda package manager.

There is one at the top-level and that one is based on downloading miniconda and setting up a FHS. This one introduces some patches libsolv and conda itself to make it work in the context of nix without needing the FHS hierarchy.

After some initial discussions with some maintainers, the default conda package has now been replaced this PR, which does not depend on the FHS. @jluttine @bhipple

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@EricTheMagician
Copy link
Contributor Author

EricTheMagician commented Mar 17, 2024

@SomeoneSerge
Thanks for the thorough review.

I've made all of the changes.
I've also made a breaking change to replace the old conda that needed the FHS with the one from this PR.

Copy link
Contributor Author

@EricTheMagician EricTheMagician left a comment

Choose a reason for hiding this comment

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

I've made all the updates.

I've updated the default libsolv library tonow build with conda as a default.
Regarding the language in the release notes: I don't have any preference. Both are fine with me. I've updated them with your suggestions.
pythonImportChecks: There was one package (conda-libmamba-solver) that I could not add it as it would create a circular dependency. I left a comment about it.

@@ -423,6 +423,10 @@ The pre-existing [services.ankisyncd](#opt-services.ankisyncd.enable) has been m

- The `erlang-ls` package no longer ships the `els_dap` binary as of v0.51.0.

- The `conda` package has been updated to 24.1.2 and is now built from source. This package no longer relies on entering a shell to install miniconda but running `conda init` is still required.
By default, conda environments will be installed in `~/.conda/envs` and conda packages will be cached in `~/.conda/pkgs`
This can be overriden at build time by modifying the `defaultEnvPath` and `defaultPkgPath` attributes respectively or at run time by setting the `CONDA_ENVS_PATH` and `CONDA_PKGS_DIRS` environment variable6.
Copy link
Contributor

Choose a reason for hiding this comment

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

variable6

Comment on lines 59 to 66
souceRoot = "${src.name}/libmambapy";

# patch needed to fix setuptools errors
# see these for reference
# https://stackoverflow.com/questions/72294299/multiple-top-level-packages-discovered-in-a-flat-layout
# https://github.com/pypa/setuptools/issues/3197#issuecomment-1078770109
postPatch = ''
substituteInPlace libmambapy/setup.py --replace-warn "setuptools.setup()" "setuptools.setup(py_modules=[])"
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment about ordering the attributes applies: sourceRoot and {pre,post}Patch go right after src, before .*Inputs

Comment on lines 43 to 44
] ++ lib.optionals withConda [
(lib.cmakeBool "ENABLE_CONDA" true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
] ++ lib.optionals withConda [
(lib.cmakeBool "ENABLE_CONDA" true)
(lib.cmakeBool "ENABLE_CONDA" withConda)

homepage = "https://github.com/mamba-org/mamba";
license = lib.licenses.bsd3;
maintainers = [ lib.maintainers.ericthemagician ];
platforms = lib.platforms.all;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = lib.platforms.all;

nixpkgs has not really python support outside of the default linux + darwin, so we might as well ommit it like in many other packages.

];

buildInputs = [
libmamba
Copy link
Contributor

@SomeoneSerge SomeoneSerge Apr 20, 2024

Choose a reason for hiding this comment

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

... different python package sets ... https://github.com/NixOS/nixpkgs/pull/296461/files#r1573356428

(libmamba.override { python3Packages = pythonPackages; }) with pythonPackages taken from callPackage

As brought up by @SuperSandro2000, the current version is broken for python3XXPackages != python3Packages:

nix path-info --recursive github:EricTheMagician/nixpkgs/conda#python310Packages.libmambapy --derivation | grep python3-
/nix/store/xg1ykawijjmpnky86808jb8kgkr64yl7-python3-minimal-3.11.9.drv
/nix/store/rxkj7xkj1wyzqvzzs2x564bx07b0nz0y-python3-minimal-3.11.9.drv
/nix/store/h1lywp34xgl1i5ilsddnqyi327yihzi7-python3-minimal-3.11.9.drv
/nix/store/wspwq9dgw9ck99dymf67sd3j1g1r4rnr-python3-3.11.9.drv
/nix/store/z60c5ran5vqkva3y0d1d5hxzykrw8pfq-python3-3.10.14.drv
/nix/store/ryvpn8k6cv9mjd5g8d3w26zsjqy92m6a-python3-3.11.9-env.drv

(the above should only list 3.10, not 3.11

@EricTheMagician
Copy link
Contributor Author

I've found some usability issues: while the basic functionality does work, I think the patch for conda I added makes it really fragile. Setting the environment variable PYTHONPATH breaks the conda executable, which brings it full circle to this comment: #296461 (comment)

Comment on lines +16 to +47
- if context.dev:
- return {
- "CONDA_EXE": sys.executable,
- # do not confuse with os.path.join, we are joining paths with ; or : delimiters
- "PYTHONPATH": os.pathsep.join(
- (CONDA_SOURCE_ROOT, os.environ.get("PYTHONPATH", ""))
- ),
- "_CE_M": "-m",
- "_CE_CONDA": "conda",
- "CONDA_PYTHON_EXE": sys.executable,
- }
- else:
- bin_dir = "Scripts" if on_win else "bin"
- exe = "conda.exe" if on_win else "conda"
- # I was going to use None to indicate a variable to unset, but that gets tricky with
- # error-on-undefined.
- return {
- "CONDA_EXE": os.path.join(sys.prefix, bin_dir, exe),
- "_CE_M": "",
- "_CE_CONDA": "",
- "CONDA_PYTHON_EXE": sys.executable,
- }
+ import sys
+ return {
+ "CONDA_EXE": sys.executable,
+ # do not confuse with os.path.join, we are joining paths with ; or : delimiters
+ "PYTHONPATH": os.pathsep.join(
+ [CONDA_SOURCE_ROOT, os.environ.get("PYTHONPATH", "")] + [path for path in sys.path if "site-packages" in path]
+ ),
+ "_CE_M": "-m",
+ "_CE_CONDA": "conda",
+ "CONDA_PYTHON_EXE": sys.executable,
Copy link
Contributor

@SomeoneSerge SomeoneSerge Apr 23, 2024

Choose a reason for hiding this comment

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

while the basic functionality does work, I think the patch for conda I added makes it really fragile. Setting the environment variable PYTHONPATH breaks the conda executable, which brings it full circle to this comment:

Could you walk us through the patch? When is context.dev true? Is the difference that upstream sets a clean PYTHONPATH that only refers to the conda environment and your patch propagates the dependencies of the conda script itself (sys.path)? Why was that required?

@EricTheMagician
Copy link
Contributor Author

I've reduced the scope for now.
I'm going to submit a different PR for replacing the top-level conda package when that is ready.
The rest of the changes are still valid for this conda package update

Copy link
Contributor

@SomeoneSerge SomeoneSerge left a comment

Choose a reason for hiding this comment

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

I'm going to submit a different PR for replacing the top-level conda package when that is ready.

Sounds like a good decision. @SuperSandro2000 would you unpin your "change request"?

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I've just pushed the last little change myself, LGTM

@SuperSandro2000
Copy link
Member

@ofborg build python311Packages.conda python312Packages.conda

@SuperSandro2000 SuperSandro2000 merged commit c0ae7ef into NixOS:master Apr 26, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants