-
Notifications
You must be signed in to change notification settings - Fork 168
Provide an option to ovewrite the existing directory by default on Unix #982
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
Conversation
I would love to provide my users with the ability to do this. We use constructor to "create a relocatable installer" and we don't expect our users to use for conda, but for the "remaining attached payload".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from that some status checks need to pass still and my other comments, I'm not sure I understand the use case of this.
The way I understand the PR description, you have installers that just have a payload without using conda
(or mamba
) to install packages, is that correct? For those installers, you just want to extract the payload into a directory by default? This would overwrite existing files with the same name and leaving old files behind, leaving a mix of old an new installation.
In my opinion, the -f
flag is dangerous because the conda
installation clobbers over the existing environments without any checks. So, I'm not sure if providing this option unchecked by default is a safe thing to do.
My concern is that we are expanding on a feature here that I think is more broken than functional. So, I would like to understand more about the use case and why users can't run the -f
option instead.
constructor/shar.py
Outdated
header_template = info.get("header_template", None) | ||
if header_template is None: | ||
template = read_header_template() | ||
else: | ||
header_template = str(header_template) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where header_template
is used nor do I see info["header_template"]
defined anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, longer answer below.
{%- else %} | ||
-e error if install prefix already exists | ||
{%- endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -e
option currently only appears in the help text, but doesn't do anything nor can it be used. It needs to be parsed below with the other options.
Only affects `.sh` installers. If `True`, the installer will remove the | ||
existing installation without prompting the user. If set, the user can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the installer will remove the existing installation without prompting the user.
The installer does not perform any removal operations. It will just clobber over the existing installations. So, the final installation directory contains a mix of new and old installations. See also #679.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this is a bug. yeah, i thought that -f
would remove, then overwrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bug in my understanding.
Co-authored-by: Marco Esters <mesters@anaconda.com>
We will typically bundle in conda/mamba into the installer but they aren't for the user to use to create virtual environment. They are to "self update". But self updates can be messy with large dependency changes.
I was hoping to first
I think I misunderstood. I thought they would I understand the dangers. These are machines that "we manage" and I think there is little difference between
and
if we tell people to "blindly follow instructions". In both cases they would "force" the install. As to why there was some I have the skills to fork it, but I generally think that a bit of customization on linux side would be nice. Perhaps even to add that In an othre world, I would have a minimal installer, 50-200MB, that would then "detect the platform, hardware specifics", and install the real optimized payload, perhaps 2-3GB large. |
Thank you for the detailed answer! I think there are several things here that all deserve separate issues, but I will address them here.
So, you understood
That's exactly what I'm wondering, if this is that much of a UX improvement that we want to maintain the extra logic with the I'd be curious about @jaimergp's opinion here, too. The
We have that for Windows already, so I don't see a reason to reject this for SH installers. That would be a separate PR though. I'd really like to see that wish list, so if you could create issues for each item, that would be amazing!
We could get partially there by allowing online installations, but that's a separate issue. Could even be environment-specific. In principle, this would only require removing the |
ok i'll try to gather my thoughts. for the "force" flag, i'm happy to implement it with my "understanding", but do you have an idea for the "flag", would it "-g"????? or "-o"???? let me know. |
Let's discuss this in an issue. I do believe that what you describe is actually what |
Provides an option for installers to be created with the
-f
option by default.I would love to provide my users with the ability to do this.
We use constructor to "create a relocatable installer" and we don't expect our users to use for conda, but for the "remaining attached payload".
Checklist - did you ...
news
directory (using the template) for the next release's release notes?