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

Add Windows CI coverage #277

Merged
merged 2 commits into from
Feb 13, 2021
Merged

Conversation

phschiele
Copy link
Contributor

@robertmartin8 As discussed in #273, this PR adds Windows coverage to the CI.

Some remarks:

  • When creating this PR, I ran into this issue with cvxpy, which was resolved by bumping the version. This an immediate example of how this coverage can help spot/resolve issues on other platforms.
  • If you think 3 Windows runners are too much, one could simply reduce the Windows coverage to Python 3.8 only (and maybe add one MacOs runner instead).
  • When bumping the cvxpy version, I realized that there seem to be multiple (conflicting) version specifications, e.g. in pyproject.toml, requirements.txt, pipfile, which I did not touch (yet). You probably have e.g. some compatibility reasons to keep those around. In general, I think it makes sense to (i) reduce the number of config files and (ii) have one source of truth and create the other files based on that. E.g. poetry can create a requirements.txt based on the pyproject.toml. Would be cool if you could share some of your thoughts here (this can also be moved to a separate issue if you like).

@robertmartin8
Copy link
Owner

@phschiele Packaging has always been an area for improvement. There are actually four dependency specifications:

  1. pyproject.toml
  2. requirements.txt
  3. pipfile
  4. setup.py

pyproject.toml is the one that I actively update and consider to be the ground truth. For the sake of best practices, it is what I recommend people use.

requirements.txt is important for people who want to use the code locally (though pip install -e is probably the best way to go). When I was setting up poetry, I couldn't find a way to create requirements from pyproject, but I'd love to be able to do that.

I added pipfile reluctantly in response to #71. I really don't know much about pipenv.

I think setup.py can be useful to have as a fallback – when other installation methods fail, cloning then python setup.py can often be a robust last resort. The key point about setup.py is that it supports python 3.5, for users who need it (though as we discussed recently, I have dropped official support).

@phschiele
Copy link
Contributor Author

@robertmartin8 Thanks for the detailed explanation. I think it makes sense to have pyproject.toml as the source of truth.

I am not an expert on packaging, and often read conflicting suggestions for best practices.
The requirements for the packaging seem to be:

  • have a single source of truth, which is easy and is convenient to use (i.e. poetry)
  • support as many different ways of installation as possible, ideally with little to no manual effort (while some conventions are being developed to have compatibility out of the box)

I was in the process of writing my views on the current file types present, but the more I tried out locally, the more obscure it got.
There are some ways to generate (some of) these files directly from poetry, like:

poetry export -f requirements.txt --output requirements.txt --without-hashes

or

poetry build
tar -xvf dist/*.tar.gz --wildcards --no-anchored '*/setup.py' --strip=1

This should be enough to cover most cases (I think pipenv also has an install -e . option, which works with setup.py).
Not sure if these automatic conversions are ideal, though.

I eventually found dephell which seems to be a quite handy tool for such tasks.
After installing (curl -L dephell.org/install | python3), I was able to generate all three fallback files, which looked quite good:

dephell deps convert --from pyproject.toml --to requirements.txt
dephell deps convert --from pyproject.toml --to Pipfile
dephell deps convert --from pyproject.toml --to setup.py

Sadly, the tool seems to be no longer actively developed.

So in summary, I think it would be ideal to have one config file. If that's not possible for compatibility reasons, the second-best option would be to have the same contents in all config files, enforce by a single small script that could be added to the development/publishing workflow.

Do you think these experiments go into the right direction?

@robertmartin8
Copy link
Owner

@phschiele dephell looks prety cool. I'll try and use that to generate requirements.txt and pipfile (though requirements.txt is much more important to me than pipfile).

As for setup.py, I will leave it the same for the poor py3.5 users out there – life must be hard enough for them!

@robertmartin8 robertmartin8 changed the base branch from master to v1.4.0 February 13, 2021 07:08
@robertmartin8 robertmartin8 merged commit d721cdc into robertmartin8:v1.4.0 Feb 13, 2021
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