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

[install] How to use requirements.txt correctly #172

Closed
kvid opened this issue Oct 10, 2020 · 3 comments
Closed

[install] How to use requirements.txt correctly #172

kvid opened this issue Oct 10, 2020 · 3 comments

Comments

@kvid
Copy link
Collaborator

kvid commented Oct 10, 2020

The current contents of requirements.txt have some issues.

@formatc1702 wrote:

Please add PIL / pillow to both requirements.txt and setup.py (under install_requires=[).
[...]

I have implemented your request, but after testing it, I have some questions:

Why do you want the run-time dependencies both in requirements.txt and in setup.py when the latter seems to be enough?

  • The first line of requirements.txt contains ., and that means, AFAIK, that setup.py is executed to find dependencies specified there, and then adding the same dependencies again from the remaining lines of requirements.txt will not change anything.
  • The last line of requirements.txt contains setuptools, but if that is not already installed before running pip install -r requirements.txt, then it will fail running setup.py when processing the first line containing ..
  • Moving setuptools above the . in requirements.txt will still fail if setuptools is not already installed, probably because all dependencies are processed before running any installation.
  • Why do you really need requirements.txt in the repo at all? it does not seem to be used by the project installation instructions. Maybe the installation instructions should have an initial pip install setuptools to make sure that is installed before running setup.py?

Originally posted by @kvid in #153 (comment)

@kvid
Copy link
Collaborator Author

kvid commented Oct 10, 2020

@formatc1702 wrote in #153 (comment):

TBH, I am not too familiar with the processes behind pip, setuptools, etc.
I have seen a lot of projects using both, and this post for example, seems to say that, while there is a way to reduce the duplicate references, if is not unreasonable to keep both files. That's why, for now, I'd like to go the safe route and keep both, too.
In the future, we could open a new issue/PR to optimize the setup process, if it is deemed necessary...

I read the same post yesterday that you link to, and I concluded there are different opinions about the reason behind requirements.txt:

  • Some argue that it's used for installing everything needed for module development without installing the module, but then it doesn't make sense to include . (that does install this module).
  • Some argue to include just . and nothing else to let setup.py do the installation job, but then why store and use the requirements.txt at all?
  • Some argue to store the output from pip freeze into requirements.txt as a documentation on the exact development environment, but in this project we cannot expect all developers to use the exact same set of versions.
  • In any case it doesn't make sense to include both . and setuptools in the same requirements.txt, as far as I can see.

I will update both files when that is your request, even though I don't like this double specification of the exact same information. A new issue is a good idea, and I will probably create that later today.

Originally posted by @kvid in #153 (comment)

@formatc1702 anwered in #153 (comment):

Yes, probably there are two of these views in our source code right now, clashing.

I think the first argument, of removing . from requirements.txt is the most reasonable, so let's do that for now. Since I am unsure of any other ramifications of emptying/removing requirements.txt at this point, let's keep the duplicate dependency info for now and discuss in a new issue.
Thanks!

Originally posted by @formatc1702 in #153 (comment)

@kvid
Copy link
Collaborator Author

kvid commented Oct 12, 2020

The redundancy part of this issue was also discussed by @gstein and @n42 in PR #58.
See also the commit history:

  1. @n42 created the initial contents - a single . in PR Refactor depency handling, installation, and add a few features #9 (which was included in release 0.1)
  2. @gstein appended graphviz and pyyaml in PR Update requirements.txt #58
  3. @aakatz3 appended setuptools in PR Implement Feature/multicolor wires on refactored code #96
  4. @kvid insert pillow in PR Add optional image to connectors and cables #153 (not yet pushed)

I hope these contributors (and others) can comment my conclusions in this issue.

I vote for removing the conflict between . and setuptools before releasing v0.2, and unless anyone argue against it, removing . from requirements.txt as concluded by @formatc1702 is probably what will happen.

I would also like an answer to the question: Who needs installing everything needed for module development without installing the wireviz module itself? Why not just install the module using pip install -e . as described both in the current README and the latest draft README - there is no mention of pip install -r requirements.txt that I can find.

@gstein
Copy link
Contributor

gstein commented Oct 12, 2020

I view requirements.txt as a simple "what's needed?" .. the inclusion of "." is a bit strange, and I didn't even know that was A Thing.

IMO, I have no voice here, as I have little contribution. I like the clarity of requirements.txt, but defer to y'all on its contents.

Thank you for asking!

kvid added a commit to kvid/WireViz that referenced this issue Oct 14, 2020
As requested by the owner, it is added to both requirements.txt and
setup.py - see also issue wireviz#172 about a possible redundancy/conflict.
@kvid kvid mentioned this issue Oct 16, 2020
7 tasks
@17o2 17o2 closed this as completed in 71a1c52 Oct 11, 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

No branches or pull requests

2 participants