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

[ENH] - Ensure that the length of all environment paths <= 255 characters #649

Closed
Tracked by #650
jaimergp opened this issue Nov 2, 2023 · 4 comments · Fixed by #653
Closed
Tracked by #650

[ENH] - Ensure that the length of all environment paths <= 255 characters #649

jaimergp opened this issue Nov 2, 2023 · 4 comments · Fixed by #653
Assignees
Labels

Comments

@jaimergp
Copy link
Member

jaimergp commented Nov 2, 2023

Feature description

Comes from #588


conda packages are guaranteed to be relocatable as long as the environment prefix length is <=255 characters.

Since we can have control over that, so I am suggesting we add checks to ensure that:

  • There are checks to control the length of the base installation path. Reject those that might incur in too long paths, or inform to the user the might need to choose a different base path for the environments.
  • If the hash used in the prefix name is configurable (see ENH - Shorten hash used in environment path #611), consider that value in the above checks.

The 255 char value can also be adjusted down depending on the system, because we might need more wiggle room for Windows installations. So I'd say a MAX_PREFIX_LENGTH=255 should be default, but it can be adjusted to a value smaller than that.

Value and/or benefit

If this is not controlled, users might run into hard to diagnose bugs like

ERROR:root:[WinError 206] The filename or extension is too long: 'C:\\Users\\Aaron\\Quansight\\conda-store\\conda-store-server\\conda-store-state\\default\\99108419ad0fd922fdeff9bbc434b58d41f68e3f923a83f6a7ab19568463bc82-20230915-201619-848782-1-test\\Lib\\site-packages\\sympy\\parsing\\autolev\\test-examples\\pydy-example-repo'

, or buffer overflows, linking errors, etc.

It will also help palliating the problems with long paths in Windows, in general, where we might want to set an even lower threshold.

Anything else?

Extra explanation about relocatability:

conda packages are relocatable thanks to a neat trick: all packages are built under a 255-char long prefix. This long prefix acts as a "placeholder" that gets replaced at install-time so it matches the destination prefix.

For example: I have a binary that hardcodes (for whatever reason) a path to another executable it needs to do a certain operation. At build time, the long prefix will have something like /opt/conda-build-workspace/project-1234abcd/envs/_h_env_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeholder_placeho. At install time, after requesting conda create -p /opt/my_envs/hallo, it will be replaced with that path, and the remaining characters will be NULL, and everyone's happy.

This trick is guaranteed to work 100% AS LONG AS the target prefix is under 255 characters. In some cases, longer paths might work because the environment doesn't contain binaries (text replacement is not as constrained) that need to be patched, but it's not guaranteed.

@jaimergp
Copy link
Member Author

jaimergp commented Nov 2, 2023

One challenging aspect here might be how to migrate folks from existing installations.

@asmeurer
Copy link
Contributor

asmeurer commented Nov 5, 2023

If this is not controlled, users might run into hard to diagnose bugs like

This is the sort of error you get from a long package path on Windows. The long prefix path errors look more like

ERROR:root:PaddingError: Placeholder of length '255' too short in package /private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gn/T/pytest-of-aaronmeurer/pytest-12/test_conda_store_register_envi0/conda-store-state/pytest-namespace/35f604188f69ceb5d9e3fae2c93ffd48c2971d192f21c776e3ebfed7e1196868-20231005-205956-556312-1-pytest-name/bin/bunzip2.

(see #588 (comment))

And actually, in this case at least, conda-store didn't even print this error message, it swallowed it. I had to dig into the code to extract the failing command and run it manually to get it. So really users might see no error. Meaning it's definitely a good idea to manually test this.

@nkaretnikov nkaretnikov self-assigned this Nov 5, 2023
@nkaretnikov
Copy link
Contributor

The only simple way to do it right now, even with a smaller hash, would be adding a runtime check to build_path. The main issue is that our prefix includes the namespace name and a bunch of other variable-lengths things in build_key. I think long-term we should move to a hash-based approach, which would be fixed-sized, see #611 (comment).

For now, I'll just add a size check to build_path.

nkaretnikov pushed a commit to nkaretnikov/conda-store that referenced this issue Nov 5, 2023
nkaretnikov pushed a commit to nkaretnikov/conda-store that referenced this issue Nov 5, 2023
nkaretnikov pushed a commit to nkaretnikov/conda-store that referenced this issue Nov 6, 2023
nkaretnikov pushed a commit to nkaretnikov/conda-store that referenced this issue Nov 6, 2023
@nkaretnikov nkaretnikov moved this from New 🚦 to In Progress 🏗 in conda-store 🐍 Nov 7, 2023
@nkaretnikov
Copy link
Contributor

Status update: will be fixed by #653 and conda-incubator/conda-store-ui#332, which are being reviewed.

@nkaretnikov nkaretnikov moved this from In Progress 🏗 to In review 👀 in conda-store 🐍 Nov 12, 2023
@github-project-automation github-project-automation bot moved this from In review 👀 to Done 💪🏾 in conda-store 🐍 Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants