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

remove setuptools from run_constrained #108

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

jjhelmus
Copy link
Contributor

@jjhelmus jjhelmus commented Dec 6, 2019

Setuptools is a requirement of conda, including it in both requirements
and run_constrained can causes conda to drop in as a direct requirement.

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Setuptools is a requirement of conda, including it in both requirements
and run_constrained can causes conda to drop in as a direct requirement.
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Dec 6, 2019

Having a package in both requirements and run_constrained can cause issues. conda/conda#9337 and related issues are likely a result of having setuptools in both places.

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

Has to be fixed upstream, of course.
But having the same entry in requirements/run and requirements/run_constrained is pointless in any case, obviously => LGTM!

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Dec 6, 2019

Has to be fixed upstream, of course.

conda/conda#9485.

I've also hotfixed the current packages in defaults with AnacondaRecipes/repodata-hotfixes#59.

@msarahan msarahan merged commit 0bcc947 into conda-forge:master Dec 6, 2019
@mbargull
Copy link
Member

mbargull commented Dec 6, 2019

Upstream as in handling it in upstream code -- but fixing the upstream recipe makes sense too ;).

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Dec 6, 2019

Upstream as in handling it in upstream code -- but fixing the upstream recipe makes sense too ;).

My understanding is that when a requirement appears in both the requirements/dependencies and run_constrained/constraints section the expected behavior is to treat the requirement as optional. This was done for backward compatibility with older version of conda which do not support constrained optional dependency.

The 4.4.0 release notes have more details on this:

For backward compatibility with versions of conda older than 4.4, a requirement may be listed in both the run and the run_constrained section. In that case older versions of conda will see the package as a hard dependency, while conda 4.4 will understand that the package is meant to be optional.

My take is that conda is interpreting conda's dependency information correctly it is just incorrect. So the package needs fixing not conda itself.

@mbargull
Copy link
Member

mbargull commented Dec 6, 2019

I.... didn't even know that! I mean, it's not really intuitive, but it somewhat makes sense. I'm just baffled I didn't know... Wish we had some kind of specification document...

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Dec 6, 2019

I only had a recollection about the behavior from discussions I had with Kale about the details when 4.4.0 was coming out and I was confused on how it all was expected to work. I had to search the changelog for the specifics.

I agree completely that conda needs a specification document on expected behaviors, standard, conventions, etc.

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.

4 participants