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

Build CairoSVG 1.0.22, needed for python<3 #24

Closed

Conversation

MutantPlatypus
Copy link

@MutantPlatypus MutantPlatypus commented Sep 25, 2019

Per the CairoSVG change log, support for Python 2 was dropped at cairosvg==2.0.0. The last version that supported python<3 was 1.0.22.

This recipe also includes all optional dependencies.

Fixes Issue #22

Checklist

  • Used a fork of the feedstock to propose changes
  • 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.

1.0.22 has three optional dependencies and two required.

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

recipe/meta.yaml Outdated Show resolved Hide resolved
@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

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

@MutantPlatypus
Copy link
Author

MutantPlatypus commented Sep 28, 2019

I think I get it now. cairosvg is pure python, but its dependencies are not. I should let the package manager figure the dependencies out: We just need to tell conda-forge to build and keep 1.0.22. That way, the dependency solver will find it for python<3. If there are problems in the dependencies not being available for some OSes for python<3, the dependency solver will notice. Issues will be reported to this or the dependency feedstocks and they can be fixed in the feedstocks of the respective dependencies.

This still allows a user to try and use cairosvg==1.0.22 in a python 3 environment if they see a need to, since it's not explicitly forbidden by cairosvg.

That still leaves the question of the optional dependencies. I think we should include them when a user asks conda to install cairosvg: It is most likely to work when all of the optional deps are there. On top of that, these optional deps turned into mandatory ones in later releases, so the maintainers of cairosvg thought the features were important enough to include. It also avoids the problem of the user needing to figure out that, even though on python 3 they just have to conda install cairosvg, on python 2 they need to conda install cairosvg lxml cssselect tinycss because their code uses some feature of cairosvg that required the optional deps. What do you think? I'll force push a clean commit history to PR#23 and reopen that PR so these experiments remain available. I can't reopen a pull request after force pushing, so I'll pop it here and open it for review.

Per the CairoSVG change log, support for Python 2 was dropped at
cairosvg==2.0.0.  The last version that supported `python<3` was
1.0.22.

This recipe also includes all optional dependencies.
@MutantPlatypus
Copy link
Author

@conda-forge-admin, please rerender

@MutantPlatypus MutantPlatypus marked this pull request as ready for review September 28, 2019 18:08
@MutantPlatypus MutantPlatypus changed the title Cairosvg 1.0.22 mindeps Build CairoSVG 1.0.22, needed for python<3 Sep 28, 2019
@carlodri
Copy link
Contributor

carlodri commented Sep 30, 2019

@MutantPlatypus as far as I understand you are correct in all your observations. The question of optional dependencies is summarized here conda/conda#7502. At the moment, it is up to the maintainers to decide and I had decided to include all of them as default to avoid user headaches.

Why did you remove the optional deps here? Sorry if I missed something. Sorry, I indeed missed something! Yes, I would leave the dependencies inside the package as you did!

@MutantPlatypus
Copy link
Author

In that case, I have no further changes I would propose for this CR, it is ready for merging. In fact, it doesn't help to wait for #17: 1.0.22 did not pass it's own testing in its repo. I wouldn't expect many of the tests being proposed as a solution to that to pass, either.

@MutantPlatypus
Copy link
Author

@conda-forge-admin, please rerender

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I tried to re-render for you, but it looks like there was nothing to do.

@MutantPlatypus
Copy link
Author

Hi @carlodri, thanks for your support. I learned a lot trying to get this going. I have a couple of other discussions going so it might be a little confusing, but this PR is ready for review separate from other issues and discussions. I even ran this build in both WSL and Windows 10 x86_64 and all SVGs render successfully.

If I have time I would like to change the tests from shell commands to a Python script so it's easier to run on other OSes, then I would like to find out exactly which dependency is causing just one test to fail if I don't pin this to python<3. (It also takes forever for the dependency solver to finish if it's not pinned to py2k.) And also get both 1.0.22 and main testing on all OSes. But I can do that in later PRs.

@MutantPlatypus MutantPlatypus force-pushed the cairosvg_1.0.22_mindeps branch from e0aa550 to b7887a4 Compare October 3, 2019 02:08
@carlodri
Copy link
Contributor

Hi @MutantPlatypus, is this PR still relevant to you? Or can we close it?

@carlodri
Copy link
Contributor

carlodri commented Aug 7, 2023

Closing due to inactivity, please reopen if relevant.

@carlodri carlodri closed this Aug 7, 2023
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