8000 Provide an option to ovewrite the existing directory by default on Unix by hmaarrfk · Pull Request #982 · conda/constructor · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Closed
wants to merge 7 commits into from

Conversation

hmaarrfk
Copy link
Contributor
@hmaarrfk hmaarrfk commented May 1, 2025

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 ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@hmaarrfk hmaarrfk requested a review from a team as a code owner May 1, 2025 16:57
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 1, 2025
@github-project-automation github-project-automation bot moved this to 🆕 New in 🔎 Review May 1, 2025
@hmaarrfk hmaarrfk force-pushed the force_by_default branch from bb44ed0 to 511c80f Compare May 1, 2025 17:05
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".
@hmaarrfk hmaarrfk force-pushed the force_by_default branch from 511c80f to b0426da Compare May 1, 2025 17:11
Copy link
Contributor
@marcoesters marcoesters left a 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.

Comment on lines 113 to 117
header_template = info.get("header_template", None)
if header_template is None:
template = read_header_template()
else:
header_template = str(header_template)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, longer answer below.

Comment on lines +111 to +113
{%- else %}
-e error if install prefix already exists
{%- endif %}
Copy link
Contributor

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.

Comment on lines +244 to +245
Only affects `.sh` installers. If `True`, the installer will remove the
existing installation without prompting the user. If set, the user can
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@github-project-automation github-project-automation bot moved this from 🆕 New to 🏗️ In Progress in 🔎 Review May 7, 2025
Co-authored-by: Marco Esters <mesters@anaconda.com>
@hmaarrfk
Copy link
Contributor Author
hmaarrfk commented May 8, 2025

n, you have installers that just have a payload without using conda (or mamba) to install packages, is that correct?

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.

For those installers, you just want to extract the payload into a directory by default?

I was hoping to first rm -rf then to install there yes.

In my opinion, the -f flag is dangerous because the conda installation clobbers over the existing environments without any checks.

I think I misunderstood. I thought they would rm -rf first.

I understand the dangers. These are machines that "we manage" and I think there is little difference between

./use_our_installer.sh -f

and

`./use_our_installer.sh

if we tell people to "blindly follow instructions". In both cases they would "force" the install.


As to why there was some template_header code, as I was going through this, there were some other modifications on my wish list. I was going to suggest after seeing what you thought of this, to simply have an option to specify a new header.sh template that we would control outside of constructor.

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 rf -rf ${HOME}/user_prefix to help with the install process.


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.

@hmaarrfk hmaarrfk changed the title Provide an option to override the existing directory by default on Unix Provide an option to ovewrite the existing directory by default on Unix May 8, 2025
@marcoesters
Copy link
Contributor

Thank you for the detailed answer! I think there are several things here that all deserve separate issues, but I will address them here.

I was hoping to first rm -rf then to install there yes.

So, you understood -f to be a "force re-install" option? That's a very reasonable assumption and much cleaner than what we have now, in my opinion. It would be a pretty big UX change, but I'm not sure how often that flag is used or how many people have the same understanding you had. That's a separate PR though.

These are machines that "we manage" and I think there is little difference between

./use_our_installer.sh -f

and

./use_our_installer.sh

if we tell people to "blindly follow instructions". In both cases they would "force" the install.

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 -e flag? This particular feature doesn't look too bad, but I'm wondering if this is definitely a one-off or if we would need to support similar behavior with other (or new) flags, too, and/or support the same ability to customize with PKG and EXE installers.

I'd be curious about @jaimergp's opinion here, too.

The -e option will have to be implemented in this PR, including integration tests.

As to why there was some template_header code, as I was going through this, there were some other modifications on my wish list. I was going to suggest after seeing what you thought of this, to simply have an option to specify a new header.sh template that we would control outside of constructor.

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!

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.

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 --offline flag when installing packages.

@hmaarrfk
Copy link
Contributor Author

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.

@hmaarrfk hmaarrfk closed this May 16, 2025
@github-project-automation github-project-automation bot moved this from 🏗️ In Progress to 🏁 Done in 🔎 Review May 16, 2025
@marcoesters
Copy link
Contributor

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 -f should do (even though it doesn't right now), so we don't need to find a new flag. It just needs to be implemented in a way that's backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

3 participants
0