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

Fix dependency conflicts #1179

Merged
merged 11 commits into from
Jan 24, 2023
Merged

Fix dependency conflicts #1179

merged 11 commits into from
Jan 24, 2023

Conversation

bonjourmauko
Copy link
Member

Fixes #1176

Bug fix

  • Do not install dependencies outside the setup.py
    • Dependencies installed outside the setup.py are not taken into account by pip's dependency resolver.
    • In case of conflicting transient dependencies, the last library installed will "impose" its dependency version.
    • This makes the installation and build of the library non-deterministic and prone to unforeseen bugs caused by external changes in dependencies' versions.

Note

A definite way to solve this issue is to clearly separate library dependencies (with a virtualenv) and a universal dependency installer for CI requirements (like pipx), taking care of:

  1. Always running tests inside the virtualenv (for example with nox).
  2. Always building outside of the virtualenv (for example with poetry installed by pipx).

Moreover, it is indeed even better to have a lock file for dependencies, similar to what is it proposed in #1015 (with pip freeze) or with tools providing such features (poetry, pipenv, etc.).

@coveralls
Copy link

coveralls commented Dec 19, 2022

Coverage Status

Coverage: 77.659% (-0.004%) from 77.664% when pulling 4f6ee1f on fix-twine-flake8-deps into f7529a2 on master.

@bonjourmauko bonjourmauko requested review from eraviart and benjello and removed request for eraviart December 19, 2022 21:20
@bonjourmauko bonjourmauko force-pushed the fix-twine-flake8-deps branch 2 times, most recently from 8be9f49 to 180f350 Compare December 19, 2022 21:23
Copy link
Member

@benjello benjello 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 to me. But I am reluctant to go beyond comment and approve this PR since I am not an expert and I already done enough mistakes lately to take the risk so early ;-)

@bonjourmauko
Copy link
Member Author

Would be great if you could test it @eraviart . I did test it on the New Zealand's package and seems to work fine.

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Dec 26, 2022

Thanks @eraviart . As I elaborate in #1176 (comment), it does not make sense to me to yank 38.0.2 as it actually unblocks the problem introduced in 35.11.1 regarding importlib-metadata. Because we've already released several majors since then, it doesn't seem practical to yank all of these versions either, unless there is a consensus to do so. Therefore, I'll modify the CHANGELOG of this PR to reflect that, by recommending using this version, removing the has been yanked part.

@bonjourmauko
Copy link
Member Author

Changelog updated to reflect that 38.0.2 actually restored a working publication.

@bonjourmauko bonjourmauko merged commit fad5f69 into master Jan 24, 2023
@bonjourmauko bonjourmauko deleted the fix-twine-flake8-deps branch January 24, 2023 15:08
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.

Missing importlib_metadata requirement
4 participants