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

Allow tests to pass without LibYAML and add LibYAML installation instructions #45

Closed
GraemeWatt opened this issue Feb 25, 2022 · 2 comments · Fixed by #47
Closed

Allow tests to pass without LibYAML and add LibYAML installation instructions #45

GraemeWatt opened this issue Feb 25, 2022 · 2 comments · Fixed by #47
Labels

Comments

@GraemeWatt
Copy link
Member

I ran pytest testsuite on the master branch and was surprised to see some tests fail. This turned out to be because I was running on a new macOS laptop without LibYAML installed. There seem to be small differences in the error messages between pure-Python YAML and LibYAML. For example,while parsing a flow mapping gives expected ',' or '}', but got '<scalar>' in pure-Python YAML but did not find expected ',' or '}' with LibYAML. The tests assume the latter. Maybe the tests could be modified to work with both pure-Python YAML and LibYAML so that developers don't need to install LibYAML.

Installing PyYAML with LibYAML enabled also turned out to be non-trivial on macOS, so it could be documented in the installation instructions. After installing LibYAML using Homebrew with brew install libyaml, PyYAML needs to be built by linking with the LibYAML C bindings:

LDFLAGS="-L$(brew --prefix)/lib" CFLAGS="-I$(brew --prefix)/include" pip install --global-option="--with-libyaml" --force pyyaml

Another minor issue with the installation instructions is that in zsh the command pip install --upgrade -e .[tests] needs additional quote marks: pip install --upgrade -e ".[tests]".

@GraemeWatt
Copy link
Member Author

I guess what's needed is to run the tests using yaml.SafeLoader and yaml.SafeDumper, then run the tests again if yaml.CSafeLoader and yaml.CSafeDumper can be imported. This would avoid the need to test with different operating systems in the CI. It looks like PyYAML is already built with LibYAML for macOS in the CI. We could then remove # pragma: no cover from the lines importing yaml.SafeLoader and yaml.SafeDumper.

@alisonrclarke
Copy link
Contributor

Surprised at how hard it is to install pyyaml without libyaml - I can't get it working locally! I'll do as you suggest, using an environment variable to allow disabling of libyaml in __init__.py

@alisonrclarke alisonrclarke moved this from To do to In Progress in @HEPData Mar 9, 2022
@alisonrclarke alisonrclarke moved this from In Progress to Ready for review in @HEPData Mar 10, 2022
Repository owner moved this from Ready for review to Done in @HEPData Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants