conda / menuinst

Cross platform menu item installation
https://conda.github.io/menuinst/
BSD 3-Clause "New" or "Revised" License
33 stars 41 forks source link

`.nonadmin` files in pre-menuinst v2 conda installations #150

Closed jaimergp closed 9 months ago

jaimergp commented 10 months ago

This is an issue for both constructor and menuinst I guess, but I'll keep it here because I think it has to do more with menuinst (even if the fix is in constructor proper).


The context:

The problem:

Possible solutions:


I feel we need to rethink how this is approached outside Windows, specially because HPC users will probably run Linux and in those setups you can find system-wide conda installations with per-user environments that should NOT ask for superuser permissions to process their own shortcuts.

Maybe it's all way simpler: if the installation is running with superuser permissions already, then install to an "All users" location. If it's not, we'll install to current user directories. But we should not depend on the state of the $CONDA_ROOT folder.

Thoughts? Tagging a few folks that have shown interest in the project or the superuser elevation issue before: @aganders3 @mrclary @marcoesters. Thanks!

mrclary commented 10 months ago
  • Add a post-link.sh script to the menuinst v2 package to add the .nonadmin file to $CONDA_ROOT in Unix systems. The logic would be something like "if it's not there, try to add it; if we could do it, it was a nonadmin install; if we failed, that's ok, it was an admin install".

Does any other package use .nonadmin? Or does constructor add this for the sole benefit of menuinst?

  • Change how we approach the "superuser permissions needed" on Unix systems. Maybe we can ignore the .nonadmin file and instead check whether the CONDA_ROOT path is in a system-wide location (/opt, /usr/local...), outside of the current user home (~/), while handling if we are sudo already or not. This might have a few rough edges though; e.g. what if the user installed to a non-standard location like /my-stuff/conda. Is that user-only? All users? Maybe it's world writable but, does that mean that the shortcuts I am processing should be available to all users?

Maybe we consider two possible solutions:

marcoesters commented 10 months ago

I personally dislike the "admin unless a file has been found" approach and would rather see a ".admin" file instead. But that may be too much of a breaking change since it also affects how constructor works.

Adding a user|all option is a good idea in either case.

The tricky part for Linux is indeed an HPC environment where there is an all-users installation but every user has a shortcut that they can remove. I wonder if a check like is_sudo or UserID == FileOwnerID could be a solution here.

Put .nonadmin in the package environment, not necessarily the root environment.

This could be a promising approach, but which package environment? The .nonadmin file is created by constructor, so it needs to be in a place that both constructor and menuinst can easily find.

jaimergp commented 10 months ago

Provide user input to menuinst specifying user|all user treatment. This would allow users to dictate behavior under in any situation.

The problem is that most of the time menuinst is called by conda behind the scenes; it's not the user dictating that directly. So we need some kind of heuristic to decide without user intervention.

Put .nonadmin in the package environment, not necessarily the root environment.

That's promising, but it's breaking the previous assumptions for Windows (where root env is checked). Maybe we would check package env first and then only base if absent, but since absence could be both meaningful (is admin) and lack of signal (we didn't try to establish whether admin is needed or not), it'll be ambiguous.

The more I think about it the more I believe that in Unix systems we can get away with a different strategy. In Windows, requesting super user permissions is more involved than prepending sudo to the command. It's usually done from the UI, right clicking on the shortcut, or pre-invoking a CMD instance with admin permissions. So it's way more cumbersome and hence why I think previous engineers working on it decided for the file sentinel for auto-elevation.

On Unix, users could just rerun the command with sudo in-front if they reaaally need an all-users shortcut. If they are installing something for all users, that probably requires sudo anyway at that point. So I'd say we check for the user id and if it's root we go for all users; otherwise, we default to the current user location.

I wonder if a check like is_sudo or UserID == FileOwnerID could be a solution here.

Yea, this (or a similar variation) is essentially what I think we should do.

mrclary commented 10 months ago

Provide user input to menuinst specifying user|all user treatment. This would allow users to dictate behavior under in any situation.

The problem is that most of the time menuinst is called by conda behind the scenes; it's not the user dictating that directly. So we need some kind of heuristic to decide without user intervention.

Conceded.

Put .nonadmin in the package environment, not necessarily the root environment.

That's promising, but it's breaking the previous assumptions for Windows (where root env is checked). Maybe we would check package env first and then only base if absent, but since absence could be both meaningful (is admin) and lack of signal (we didn't try to establish whether admin is needed or not), it'll be ambiguous.

What if we disambiguated by always creating a file, i.e. .nonadmin and .admin. If there is no file, then check base environment (for old paradigm), otherwise it is affirmatively declaring to create shortcuts for all users or just this user.

jaimergp commented 10 months ago

What if we disambiguated by always creating a file, i.e. .nonadmin and .admin.

That can work for new environments, but we have to figure out a way for old environments too.

I am going to start prototyping something in a PR and we'll see how ugly that looks :D

jaimergp commented 10 months ago

156 is now ready for review implementing the discussed approach (sans .admin markers).