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

Set minimum required version of dependencies #280

Merged
merged 26 commits into from
Dec 21, 2021
Merged

Set minimum required version of dependencies #280

merged 26 commits into from
Dec 21, 2021

Conversation

leouieda
Copy link
Member

@leouieda leouieda commented Dec 20, 2021

Establish the minimum supported version of run-time and optional dependencies (see fatiando/community#40). Support all releases from the past 2 years with a minimum of 2 minor versions.

Needed to do some maintenance work on our CI and packaging setup to enable this:

  1. Moved the requirements into separate files in the env folder.
  2. Moved package metadata to setup.cfg and pyproject.toml so that we can programmatically convert run-time dependencies into a requirements.txt file with tools/export_requirements.py
  3. Made a tools/oldest_requirements.py that converts >= into ==. Could have been a sed call but getting this to work on Mac and Linux was a huge pain. The Python script is easier.
  4. Simplify the CI scripts to use the requirements files and tools scripts to build their list of packages.

I also ended up moving license_notice.py to tools since we have more of them now.

Almost everything worked perfectly with the oldest versions out of the box. The only modification I had to make was removing some of the xxhash algorithms from the tests since they were introduced in a later version. This shouldn't be a problem because the underlying implementation is the same either way.

Also addresses fatiando/community#42

Reminders:

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst and the base __init__.py file for the package.
  • Write detailed docstrings for all functions/classes/methods. It often helps to design better code if you write the docstrings first.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.
  • Add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.

Move the requirements into separate files in the `env` folder and get
run-time dependencies directly from `setup.py`. Simplify the CI
configuration to install from these files.
This was being masked by tqdm and paramiko always being installed
together
Needed this to be able to capture the dependencies using the
configparser library.
For compatibility with recent setuptools. It's from Jan 2020 so it
counts.
It was only introduced in a later version. We're OK not testing it here
since we check the other hashes already.
@leouieda leouieda marked this pull request as ready for review December 20, 2021 11:08
@leouieda
Copy link
Member Author

@dopplershift this is largely based on our twitter chat the other day and the MetPy configuration. Would you be able to give this a quick look? No rush on this, whenever you have a spare moment.

@leouieda leouieda added this to the v1.6.0 milestone Dec 20, 2021
Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Looks good!

And to be clear, users don't even need to care about the requirements.txt thing--unless it causes installation problems due to overly tight pinning. It's something I run into while reviewing conda-forge recipes.

Comment on lines 39 to 40
"xxh128": "0267d220db258fffb0c567c0ecd1b689",
"xxh3_128": "0267d220db258fffb0c567c0ecd1b689",
"xxh64": "f843815fe57948fa",
"xxh3_64": "811e3f2a12aec53f",
"xxh32": "98d6f1a2",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a version when these should start passing, instead of removing them you could wrap them with skipif--assuming you could detect what version you have.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏽 good idea! Implementing that now.

@leouieda
Copy link
Member Author

Thanks for the feedback, Ryan! It was time we modernized our build system anyway and we've forgotten to package a requirement.txt file in the past so this is good to help avoid that in the future.

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.

2 participants