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

RFC Add tox #254

Closed
wants to merge 1 commit into from
Closed

RFC Add tox #254

wants to merge 1 commit into from

Conversation

fornellas
Copy link
Contributor

@fornellas fornellas commented Oct 18, 2020

We hit environment inconsistencies from time to time tox should help with some of that.

Pending things:

  • Use same Python versions at .travis.yml, .python-version (pyenv) and tox.ini.
  • Pin dependency versions both from setup.py and tox.ini.
  • Have a way to "prepare local env": install tox & deps defined at tox.ini (eg: black) so that make format can work.
  • Make TravisCI run isort, black & others.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 18, 2020
Copy link
Member

@deathowl deathowl left a comment

Choose a reason for hiding this comment

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

Tox \o/ Yes

@deathowl
Copy link
Member

deathowl commented Oct 18, 2020

@fornellas Let's move our deps out of setup.py and into requirements.txt I can do that if you wish. So setting up local venv is much much easier. Or even better. Pipfile

@fornellas
Copy link
Contributor Author

Sure! I can rebase this on top

@facebook-github-bot
Copy link

Hi @fornellas!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@macisamuele
Copy link
Contributor

Is there still an interest on having this PR merged?
I would love the idea of having this as it simplifies testing onto different versions and ensuring consistency between CI and local test runs.

I have to admit that I find not straightforward (and often forget) the fact that testing the tool requires developers to build their venv, activate it and then run the make commands.

If there is still interest on having this, but eventually lack of bandwidth please let me know such that I can try to update the branch on top of the master branch (we no longer use travis).


A general comment on the current PR structure is related to the usage of make commands into tox. This feels a sort of antipattern to me as we are going to use tox as way to build controlled venvs and then depend on the fact that /usr/bin/make exists (which excludes the possibility of running the tests on Windows).
I would have expected to have tox responsible of creating venvs and running the commands and eventually preserving the Makefile as a sort of glue for whom is custom to make commands.

Additionally, the Contributing.md updates mention the creation of a custom venv in order to run specified tests, while it would be achievable via {posargs} (doc) in tox (so yet one less hack/manual action)

@fornellas
Copy link
Contributor Author

@macisamuele I won't have bandwidth for this, but feel free to carry this on.

Some thoughts:

  • Yes, make + venv = annoying. However, tox is not a full solution. Ideally, using Docker and having an "official dev env image" would be the best way to guarantee reproducibility.
  • Ditching make is good in general, however, please check the Makefile, as there's a fair bit of logic / sequence of steps absolutely requequired to run all tests (eg: coverage) which must be replicated.
  • There was never Windows support, as there was no such use case. However, I suppose adding a GH action for a Windows bulid would cover it.

@deathowl
Copy link
Member

I think this is still super valuable, let's carry this on and make it mergeable

@macisamuele
Copy link
Contributor

I'll be looking into it in the next days.
Considering the nature of the project (independent from the underlying OS) having and forcing the docker presence seems not needed.
If this was first configured to ensure reproducible runs I'll make sure that this would still be achieved by running tox (and makefile to glue the tox runs as the ci would do).

Still the above mentioned concern around forcing docker (which would effectively make windows and osx tests, if we enable them, not really reproducible) will be laid down during the pr.

@fornellas
Copy link
Contributor Author

I'm closing this PR as I realistically won't finish it. @macisamuele feel free to carry on :-D

@fornellas fornellas closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants