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

Require ruamel.yaml #81

Closed

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Mar 8, 2019

Switches to requiring ruamel.yaml instead of ruamel_yaml.

cc @jjhelmus

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.

Rebuild the package now that `ruamel.yaml` is required instead of
`ruamel_yaml`.
@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

jjhelmus commented Mar 8, 2019

This likely needs a tighter upper bound as conda has issues with later versions of the ruamel.yaml 0.15.x series. I believe 0.15.64 works but 0.15.87 breaks some tests.

@jakirkham
Copy link
Member Author

Does 0.15.88 and 0.15.89 also break tests or was it just 0.15.87?

@jjhelmus
Copy link
Contributor

jjhelmus commented Mar 8, 2019

I have not tested 0.15.88/89 or really anything between 0.15.64 and 0.15.87. The safest bet would be the make the upper bound <=0.15.64. A new build could relax this once a more precise upper bound is known.

I will have some time next week to test the various newer versions and hopefully fix the issues in conda around them.

@jakirkham
Copy link
Member Author

Thanks @jjhelmus. That would be very helpful :)

@scopatz
Copy link
Member

scopatz commented Apr 25, 2019

Can this be closed?

@jakirkham
Copy link
Member Author

Nope. This still needs to be done. Just haven't had time of late. Feel free to push on it if you do. 🙂

@mariusvniekerk
Copy link
Member

What is the current state of this? It would be great to merge this.

@jakirkham
Copy link
Member Author

That's a great question. I don't know. Would also be happy to see this merged.

@jjhelmus raised some concerns about conda-build's sensitivity to different ruamel.yaml patch releases. This remains unaddressed. Maybe someone with his guidance could investigate this? 🙂

@jjhelmus
Copy link
Contributor

jjhelmus commented Nov 6, 2019

AFAIK conda can use either ruamel.yaml or ruamel_yaml but is sensitive to the version. "Recent" versions were causing conda's test suite to fail so we have held off upgrading in defaults. Recent may be the >0.16 although I seem to recall some later 0.15.x release also causing the test suite to fail.

Investigation into compatible versions and updating conda so it works with the latest would be wonderful.

@jakirkham
Copy link
Member Author

It would be good to understand why Conda is sensitive so we can mitigate this (or at least raise issues upstream) to improve stability.

@mbargull
Copy link
Member

mbargull commented Nov 8, 2019

AFAIK conda can use either

Yep, conda will happily take either.
I'm in favor of this change. There is no good technical reason anymore for ruamel_yaml's existence.

However, please consider conda-forge/ruamel.yaml-feedstock#119 (comment) prior to this. This will affect conda then, too. Plus, it will have consequences for older builds of ruamel.yaml.

Recent" versions were causing conda's test suite to fail

Doing a quick ruamel_yaml search on conda's PRs, I found conda/conda#8240 (comment) with the mentioned failure being at https://ci.appveyor.com/project/ContinuumAnalyticsFOSS/conda/builds/22201086/job/5xdk2m3rkti9rihd#L1131 with its code being at https://github.com/conda/conda/blob/b9edc96eaea49c4608d13464867c3e2b76a63601/tests/test_create.py#L1038-L1046. That one is totally fine to fail. It is conda's tests testing that it does not adhere to the YAML spec wrt. duplicate keys.
Duplicate keys are generally not allowed in YAML (https://yaml.org/spec/1.2/spec.html#id2759572), even in the older YAML 1.1, however the latter had a (now removed) recommended non-failing error handling: "In such a case the YAML processor may continue, ignoring the second key: value pair and issuing an appropriate warning.".
The conda test made sure that a latter key overrides prior ones, i.e., it is not ignored as per spec.
To fix conda, we can just fix that test case. IMO, no need for a "deprecation phase" since ruamel.yaml put out warnings for a long time since 0.15.1 on: https://bitbucket.org/ruamel/yaml/src/0.15.1/CHANGES.

@jakirkham
Copy link
Member Author

Thanks for putting all that info together @mbargull !

@scopatz
Copy link
Member

scopatz commented Nov 8, 2019

So is this OK to merge?

@mbargull
Copy link
Member

mbargull commented Nov 8, 2019

Not yet. Things to do:

  1. Either restrict the version to <=0.15.64 or check whether there are other incompatibilities between conda and ruamel.yaml>0.15.64,<0.16 (apart from the duplicate key handling I mentioned above).
  2. Preferably decide on what to do with Resolving "cyclic" dependency with ruamel.yaml.clib ruamel.yaml-feedstock#119,
    but at least be aware of Resolving "cyclic" dependency with ruamel.yaml.clib ruamel.yaml-feedstock#119 (comment) and its consequences.

Re 2.: The given solutions are not "optimal" (the one I prefer involves metadata patching which I normally shy away from, for good reasons). I would like us to reach consensus on that issue before making ruamel.yaml an ecosystem-critical requirement.

@jezdez jezdez mentioned this pull request Nov 30, 2022
5 tasks
@jezdez jezdez closed this in #181 Dec 2, 2022
@jakirkham jakirkham deleted the use_ruamel.yaml branch December 2, 2022 16:33
@jakirkham
Copy link
Member Author

Thanks Jannis! 🙏

Nice to see this resolved 😄

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.

6 participants