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

test: ensure apt *and* python c extensions work properly #1222

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iloveitaly
Copy link
Contributor

@iloveitaly iloveitaly commented Nov 13, 2024

Repost of this PR which was closed: #1103


I ran into a tricky problem:

  • I have a py project
  • The project has to execute a system command, notably apt in this case
  • stdenv.cc.cc.lib is required for numpy to work, but it breaks apt (and, more importantly, I'm assuming many other system commands) with the following error:
0.110 nix-env: /nix/store/02mqs1by2vab9yzw0qc4j7463w78p3ps-glibc-2.37-8/lib/libc.so.6: version `GLIBC_2.38' not found (required by nix-env)

This error can be fixed by reverting the mutated LD_LIBRARY_PATH from:

LD_LIBRARY_PATH=/nix/store/cq3kvw7kwz2n9c13fg6qaaj23mkm116l-gcc-12.3.0-lib/lib:/nix/store/wzm0753byf1app0xrsa97hjnfk9m1zs0-zlib-1.3/lib:/usr/lib

to:

LD_LIBRARY_PATH=/usr/lib

Did some research:

I'm new to nix. It seems like creating a bespoke shell env with LD modified is the way to go, but that wouldn't actually solve this issue since
LD_LIBRARY_PATH would be inherited by the py process and therefore passed to the shell (unless the modified LD only applies to package installation, but I'm confident that would work either.

In any case, I'm curious what folks think here. I added a failing test to work off of, but unsure of the best solution.

Docker container success assertion

In investigating this, I found that the docker container execution is not asserted to be successful. This feels like a good change
to make for all containers to avoid swallowing otherwise important errors.

@iloveitaly
Copy link
Contributor Author

The problem seems to be a mismatch between the glibc version installed in Ubuntu and used by the system packages and the glibc version installed and used by the Python Nix package

Yup, this is definitely the issue.

I'm not super comfortable globally modifying the LD_LIBRARY_PATH variable to not include any of the Nix packages

Agreed. That would cause issues in other ways.

If possible I think we should update the Ubuntu glibc version or have an configuration option to reset the LD_LIBRARY_PATH variable.

Yeah, having a shell script bundled to do this wouldn't be terrible. However, there's got to be a better way. I don't know nix enough to understand what the canonical way to handle this, but I imagine this is something that other nix users have run into before.

Copy link
Contributor

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale The pull request is stale label Nov 24, 2024
@iloveitaly
Copy link
Contributor Author

Friendly reminder about this pull request! Let me know if there's anything else needed here.

@github-actions github-actions bot removed the stale The pull request is stale label Nov 25, 2024
@coffee-cup
Copy link
Contributor

Hi @iloveitaly, I'm a little worried about removing stdenv.cc.cc.lib breaking numpy. I think that is an important package to work out of the box. Can you fix the lints so the docker tests can run and we can see if they break.

@iloveitaly
Copy link
Contributor Author

Yeah, I'm not really sure what the best solution here is. My limited knowledge of nixpkgs is working against me. Will fix the lining and see if that gets us closer.

Copy link
Contributor

github-actions bot commented Dec 8, 2024

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale The pull request is stale label Dec 8, 2024
@iloveitaly
Copy link
Contributor Author

Friendly reminder on this pull request! Let me know if there are any updates or additional changes needed.

@github-actions github-actions bot removed the stale The pull request is stale label Dec 9, 2024
@coffee-cup
Copy link
Contributor

Yes sorry @iloveitaly, haven't forgot about this. Just unsure of the best solution currently and haven't had that much time to explore solutions.

@iloveitaly
Copy link
Contributor Author

Yeah, I'm unclear as well. Is there anyone tied to the project that has deeper nix experience that could weigh in? I imagine this problem has come up quite a bit in the past.

@JakeCooper
Copy link
Contributor

Hmm could we dynamically set the LD_LIBRARY_PATH based on the providers?

You'd almost need like, a hash map, and then each provider could either create an entry, or blank an entry, during a phase. And then you'd join it all into the LD_LIBRARY_PATH

Or maybe have an install phase run as a "pre-phase" execution?

Like in theory you'd solve this by doing all your apt stuff, modifying LD_LIBRARY_PATH, and then doing "pip install" right?

@coffee-cup
Copy link
Contributor

Hmm could we dynamically set the LD_LIBRARY_PATH based on the providers?

We already do this with the nixLibs build plan config https://nixpacks.com/docs/configuration/file#nix-libraries

But the problem in the failing code example provided is that both numpy and apt are used.

@coffee-cup
Copy link
Contributor

I wonder if we could build a base image of debian where the glibc version matches the glibc version in Nix

@coffee-cup
Copy link
Contributor

What are the use cases for requiring apt in Python? This Nix way would be to install everything with Nix packages and then the glibc versions would all be compatible. The problem you are trying to mix non-Nix with Nix packages together.

One option would be for us to install Python not using Nix at all (and with something like asdf)

@iloveitaly
Copy link
Contributor Author

I like this idea of using mise or asdf to install python. It would be really interesting to use mise across all providers.

I ran into this specifically when launching an interactive she'll and debugging a container issue locally. However, I ran into this issue on tools beyond apt which means that users could run into this when shelling out to a CLI binary.

I like the map-based modification, although I don't have a strong idea of what impacts that would have on the rest of the system. I just don't have enough incentive right now to learn nix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants