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

meta.yaml: upgrade to 2.0.5; adds pandoc to dep and tests they work t… #8

Merged
merged 4 commits into from
Dec 16, 2020
Merged

Conversation

ickc
Copy link
Contributor

@ickc ickc commented Dec 14, 2020

…ogether

Checklist

  • Used a personal 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.
  • upgrade to 2.0.5
  • adds pandoc to dep and tests they work together

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

@ickc
Copy link
Contributor Author

ickc commented Dec 14, 2020

@conda-forge-admin, please rerender

@ickc
Copy link
Contributor Author

ickc commented Dec 14, 2020

@kiwi0fruit, @beckermr, could you help merging this please?

recipe/meta.yaml Outdated Show resolved Hide resolved
@kiwi0fruit
Copy link

@ickc I don't think straying from pip panflute behaviour is a good idea. But pandoc is not a python dep so we might need to decide this from scratch. I'm OK with this solution as I only use panflute with conda-forge pandoc.

@kiwi0fruit
Copy link

@ickc run_constrained suggested above might be a good option indeed.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

@ickc I don't think straying from pip panflute behaviour is a good idea. But pandoc is not a python dep so we might need to decide this from scratch. I'm OK with this solution as I only use panflute with conda-forge pandoc.

The whole point of conda is to manage dependencies outside the Python ecosystem. If we just stick with pip behavior there's no point in inventing conda in the first place.

pip doesn't include pandoc is a deficiency, not a feature. See e.g. in pypandoc they packaged pandoc within the packaging in pip long ago but it was too troublesome so in the end they just wrote a download_pandoc function and ask user to run that if pandoc is absent, and it will download it automatically from pandoc's GitHub Releases.

Unless there's any other way to help users to get a matching version of pandoc and panflute, I think it is very critical to have a proper package manager to do just that for them. In the past panflute has been supporting a few recent versions of pandoc, non-matching pandoc/panflute versions wasn't a big problem. But since the 2.10-11 releases it created chaos, which exposes the critical problem in packaging. Another breaking change is likely to be coming (see jgm/pandoc-types#86), so we better be ready to prevent the next chaos.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

I think the simpler way to put it is how we classify panflute—does it optionally depends on pandoc or it won't work properly without pandoc.

While indeed you can construct the AST using pure panflute, it is very difficult to imagine a practical scenario that people aren't using pandoc when they are using panflute (e.g. as a pandoc filter pandoc has to be there, or when using convert_text after AST is constructed pandoc still has to be there...)

@kiwi0fruit
Copy link

If adding pandoc as a hard dep then it should be hard dep only on windows amd64, linux amd64 and macos amd64. In other cases there should not be pandoc dep.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

https://anaconda.org/anaconda/pandoc

Most platforms supported by conda has pandoc. The only one I can think of isn't in that list is ARM.

@beckermr
Copy link
Member

That is the wrong link. In the conda-forge channels we do not support ppc64le, aarch64 or osx-arm64.

https://anaconda.org/conda-forge/pandoc/files

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

I actually prefer having two conda recipe, one pandoc-panflute and another just panflute here, where the name pandoc-panflute indicates clearly that it depends on pandoc. But they don't seem to think it makes sense.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

@beckermr,

That is the wrong link. In the conda-forge channels we do not support ppc64le, aarch64 or osx-arm64.

It is not a wrong link. multiple channels can be mixed and since pandoc is a static binary it will be fine. Anaconda will release newer versions of pandoc later. For now if users in those arch trying to install this then the solver should choose panflute 2.0.4 that you merged recently.

@beckermr
Copy link
Member

Conda forge usually requires strict channel priority which would remove those packages from defaults from consideration from environments. Defaults and conda forge cannot be mixed freely.

@beckermr
Copy link
Member

In any case, now that you all understand the differences I'll leave it to you to figure out what you want.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

If we can't agree on that then let's make it optional. Better gets things done and makes everyone happy than spend times arguing. I will try to convince them to add pandoc-pandflute, which is preferrable to have choices and have one of them unbreakable for novice.

I'll change it tomorrow but few free to take over and merge.

@ickc
Copy link
Contributor Author

ickc commented Dec 15, 2020

@beckermr,

Conda forge usually requires strict channel priority which would remove those packages from defaults from consideration from environments. Defaults and conda forge cannot be mixed freely.

Not true either. Strict means that if it doesn't exist from the 1st channel it will fallback to the next (default).

And they can be mixed, just be careful about ABI compatibilities. For most packages they don't even have this problem. For others, they should know what they're doing already.

@ickc
Copy link
Contributor Author

ickc commented Dec 16, 2020

Ready to merge?

@kiwi0fruit
Copy link

@conda-forge-admin, please rerender

1 similar comment
@kiwi0fruit
Copy link

@conda-forge-admin, please rerender

@ickc
Copy link
Contributor Author

ickc commented Dec 16, 2020

@kiwi0fruit, there's nothing to render, it is up-to-date. I rerendered it locally:

INFO:conda_smithy.configure_feedstock:No changes made. This feedstock is up-to-date.

@kiwi0fruit kiwi0fruit self-assigned this Dec 16, 2020
@beckermr
Copy link
Member

No @ickc. Please read the documentation on channel priority here:

https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-channels.html#strict-channel-priority

With strict channel priority, packages in lower priority channels are not considered if a package with the same name appears in a higher priority channel.

Mixing defaults and conda-forge with flexible channel priority is not recommended. There is currently at least once case where the solver will return incorrect environments when using flkexible channel priority.

@kiwi0fruit kiwi0fruit merged commit 3386d92 into conda-forge:master Dec 16, 2020
@isuruf
Copy link
Member

isuruf commented Dec 16, 2020

@beckermr, that's not true. @ickc is right. strict channel priority would work

@beckermr
Copy link
Member

beckermr commented Dec 16, 2020

Ah yes because the package is on a different platform/subdir. My apologies!

@ickc
Copy link
Contributor Author

ickc commented Dec 17, 2020

Mixing defaults and conda-forge with flexible channel priority is not recommended. There is currently at least once case where the solver will return incorrect environments when using flkexible channel priority.

Probably you understand it now, just in case: You can set conda-forge as 1st, defaults as 2nd, and use strict priority. In the case you warned about, which is on arch conda-forge don't release pandoc, since it doesn't exist in conda-forge, it will falls back to defaults. However for those arch that conda-forge offers pandoc and you specified strict priority, it will choose conda-forge only.

@ickc
Copy link
Contributor Author

ickc commented Dec 17, 2020

Thanks @kiwi0fruit for merging it!

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.

5 participants