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

Only use setup.py #981

Merged
merged 5 commits into from
Apr 24, 2019
Merged

Only use setup.py #981

merged 5 commits into from
Apr 24, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 22, 2019

PR feedback of #979. We can only use setup.py instead of separating requirements files.

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

I like the change, but it is broken in some ways:

  • preset_loader (from local module config_helpers) is not installed. The conftest errors.
  • when running make install_test on a clean repo, it generates a pyspec.egg-info, something I don't have in my global git ignore, and many other non-python contributors as well. May just want to add it to the .gitignore

You can try by doing a make clean, and then a test-run. Not sure why the CI didn't catch it.

@hwwhww hwwhww force-pushed the only_setup_py branch 2 times, most recently from 00f97f5 to 05c805a Compare April 23, 2019 04:13
@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 23, 2019

@protolambda

Ready for another look!

preset_loader (from local module config_helpers) is not installed. The conftest errors.

I think CI didn't raise due to the cache. My current workaround is:

  • Having venv under test_libs/ instead of test_libs/pyspec
  • Installing config_helpers separately

IMO the better solution may be using one single setup.py for all test_libs sub packages. What do you think about it?

when running make install_test on a clean repo, it generates a pyspec.egg-info, something I don't have in my global git ignore, and many other non-python contributors as well. May just want to add it to the .gitignore

Updated .gitignore. :)

@djrtwo djrtwo merged commit 98e6a24 into pytest-use-config Apr 24, 2019
@djrtwo djrtwo deleted the only_setup_py branch April 24, 2019 17:58
@djrtwo djrtwo restored the only_setup_py branch April 24, 2019 18:00
@djrtwo
Copy link
Contributor

djrtwo commented Apr 24, 2019

I messed up and merged this into proto's branch by accident. sorry!

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 25, 2019

@djrtwo 😆 I'm not sure what happend. My understanding is:

  1. Only use setup.py #981 was merged into pytest-use-config branch (pytests use configuration system #979)
  2. pytests use configuration system #979 (pytest-use-config branch) was merged into dev
  3. Only use setup.py #981 was reverted (Revert "Only use setup.py" #991) in pytest-use-config branch

I guess you wanted to revert it in dev branch?

@djrtwo
Copy link
Contributor

djrtwo commented May 1, 2019

Yup, messed this one up..

@hwwhww hwwhww mentioned this pull request May 2, 2019
@protolambda
Copy link
Collaborator

Important comment: #1020 (comment)
Discussion continued here.

@protolambda
Copy link
Collaborator

To make sure I understand it correctly, #981 was reverted because it was merged before fully review process? And I should open another PR for it?

Mostly yes. It was reverted, because the solution breaks testing from a clean install. And although the partial solution was nice & reviewed, it didn't cover everything, and was merged prematurely.

Do you mean in CI? It seems another issue, not related to setup.py route, correct?

Well, CI testing is one thing. Agree that that can be cleaner. But we don't change it often, And I prefer having fast tests for 95% of the other PRs. The issue is installing packages doesn't fully work with setup.py as only solution: it only installs remote packages, not the ../../foobar packages listed in the requirements(-testing).txt

#981 (comment): IMO the better solution may be using one single setup.py for all test_libs sub packages. What do you think about it?
I see there's more value in organizing all dependencies in one file
Easier to manage the dependencies

Not on the same page here. When you combine packages, you mix up dependencies. Some solutions are packaged separately, as to not put unrelated code in the scope of the modules consuming the dependencies.

If we want to release it in pypi with spec release cycle

Not a fan of this honestly. IF we were looking to go full software release-cycle, we wouldn't be writing a "specification". In the software case: then also just break some of those readability-performance issues, and make it actually viable to use directly.

Not a hacky way. :)

I do like the setup.py idea, but I'm not willing to give up on strong encapsulation and can't see how a setup.py can handle the local dependencies; breaking the structure :(

Alternative solution

There is another approach, which keeps things minimal, non-duplicate, and fixes some confusion in the process of the installing when using an IDE: just use requirements.txt, and use the bare minimum setup.py. I.e. no dependencies/version/whatever listed, just enough to make it a "module".

The idea of requirements.txt fits the use-case better (imo): our end-user is working on the spec itself, and wants to change dependency versions, or add local modules for testing things. I.e. not just consuming the spec as a normal pypi user.

TLDR:

So unless there is a way to introduce local requirements into setup.py, without breaking encapsulation, and it actually works: I prefer to keep it as-is, or even minimize the setup.py complexity.

@hwwhww hwwhww deleted the only_setup_py branch May 9, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants