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

Fix uninstallation of conda-forge-ci-setup #1596

Closed
wants to merge 1 commit into from

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Feb 21, 2022

This attempts to help the bootstrap process uninstall any pre-installed version of conda-forge-ci-setup.

I've added comments to the effect where I knew how to do it in the relevant syntax.

The purpose of specifying extra packages was specifically designed in to avoid needless churn with the tensorflow package which depends on lief_dev which has not had a new release in about 8 months.

I'm really trying to contain scope creep here and not balloon the options provided in conda-forge.yml

References:
Lief releases: https://github.com/lief-project/LIEF/releases

Request to use lief dev: conda-forge/tensorflow-feedstock#189 (comment)

Interested parties: @beckermr @isuruf @xhochy

Checklist

  • Added a news entry

- cmd: conda.exe uninstall --quiet --yes --force {{ " ".join(remote_ci_setup) }}
# Uninstall the conda-forge-ci-setup package but leave its dependencies
# so that we may bootstrap the installation process
- cmd: conda.exe uninstall --quiet --yes --force conda-forge-ci-setup
Copy link
Member

Choose a reason for hiding this comment

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

This was an option so the package could be renamed. We should preserve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think we have two options:

  1. Remove the uninstall step and specify each dependency to conda-forge-ci-setup in the conda-forge.yml file. This seems quite difficult with win/osx/linux specifics.
  2. Create a new option.

To be clear, I figured that "renaming" wasn't so interesting because of the hard coded nature of the package name:

"conda_forge_ci_setup",

https://github.com/conda-forge/conda-forge-ci-setup-feedstock/blob/main/recipe/meta.yaml#L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 3rd option is to leave it as is.

The current "state" works for tensorflow, and works for conda-forge-ci-setup. This is "good enough" for me.

Copy link
Member

Choose a reason for hiding this comment

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

We actually have the version as part of the option:

"remote_ci_setup": ["conda-forge-ci-setup=3"],

So smithy has been using a mix of things.

I don't have strong feelings here, but it seems like if folks want to add extra ci setup packages, then we should have added a new optional originally called "extra_ci_setup" or similar.

In any case, most things are half-broken most of the time. I'll defer to the rest of core here.

cc @conda-forge/core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally proposed something like extra, it was rejected. I think we hadn't considered the bootstrap procedure. Selfishly speaking, i don't have many reasons to push this PR forward

Copy link
Member

Choose a reason for hiding this comment

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

That is totally fine. Let's close this one and leave things as they are. We can go back and fix later by making the conda-forge specific stuff a separate option since it is special anyways. Thank you for all of the hard work here!

Copy link
Member

Choose a reason for hiding this comment

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

xref: #1597

@beckermr
Copy link
Member

closing as stated above!

@beckermr beckermr closed this Feb 21, 2022
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.

2 participants