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

[2.11.x] marginally loosen upper bound for markupsafe #32

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Jun 22, 2022

This does not represent the upstream bounds, and leads to resolution errors where things should actually work.

Edit: see discussion below

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

@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

@h-vetinari, while you are right that the upper bound is not present upstream, you can find extensive discussion (e.g. here) about the fact that newer versions of markupsafe break jinja2. We have had to work hard to patch older versions (see here, here and here) to prevent this issue from affecting many workflows. There is definitely an incompatibility so this upper bound is needed.

@xylar xylar marked this pull request as draft June 22, 2022 06:28
@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

One more link that might be helpful: pallets/jinja#1606 (comment)

@h-vetinari
Copy link
Member Author

OK, thanks a lot for the context. Looking at the issues you linked and the repodata patches, I see that <2.1 would be sufficient. You yourself noted:

markupsafe <2.1 is neeeded to avoid the issue in:
pallets/jinja#1585
We are constraining markupsafe <2 to be on the safe side.

Is that "safe side" really necessary? In my case it does more harm than good. Would you accept a patch that restricts to <2.1? Given that these versions are not really in widespread use anymore, this should also not have a large impact anyway.

@h-vetinari h-vetinari changed the title remove upper bound for markupsafe marginally loosen upper bound for markupsafe Jun 22, 2022
@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

@h-vetinari,

Given that these versions are not really in widespread use anymore, this should also not have a large impact anyway.

I'm afraid I don't agree there. I have found a lot of packages still requiring jinja2<3, which is why this has been quite a struggle.

Is that "safe side" really necessary? In my case it does more harm than good. Would you accept a patch that restricts to <2.1?

That would work for me. It would require yet more repo patching if you want other versions to work. Before we do this, though, could you explain what you have that requires markupsafe >= 2, just so I can follow along?

@xylar xylar marked this pull request as ready for review June 22, 2022 07:13
@xylar xylar changed the title marginally loosen upper bound for markupsafe [2.11.x] marginally loosen upper bound for markupsafe Jun 22, 2022
@h-vetinari
Copy link
Member Author

I'm afraid I don't agree there. I have found a lot of packages still requiring jinja2<3, which is why this has been quite a struggle.

Fair enough. Thanks for taking care of this so thoroughly!

That would work for me. It would require yet more repo patching if you want other versions to work. Before we do this, though, could you explain what you have that requires markupsafe >= 2, just so I can follow along?

I'm currently porting some internal packages using pip dependencies to conda, and one is currently pinning the legal-to-pip & working-in-practice flask 1.1.4 & markupsafe 2.0.1, which however is uninstallable with conda-forge.

@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

I'm afraid that legal-to-pip may not be the same as not broken but I'm willing to give this a try...

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

I think this won't break anything 🤞🏻

@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

@jakirkham and @scopatz, what do you think?

@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

@nehaljwani as well, what do you think?

@nehaljwani
Copy link
Member

nehaljwani commented Jun 22, 2022

I trust your judgement, @xylar ! Feel free to merge 🙂

@xylar
Copy link
Contributor

xylar commented Jun 22, 2022

Okay, well at the risk of having some more repo patching to do later, let's see what happens...

@xylar xylar merged commit 50f60e0 into conda-forge:2.11.x Jun 22, 2022
@h-vetinari h-vetinari deleted the 2.11.x branch June 22, 2022 16:12
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