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

feat(setuptools): add find_requirements function #5648

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sharpvik
Copy link

@sharpvik sharpvik commented Apr 8, 2023

The issue

Pipenv is the de facto standard for virtual environment and dependency management in Python. Yet, Python's main package and CLI distribution mechanism is setup.py scripts. As of now, these two things are not integrated, but they can be!

I saw an issue regarding this problem somewhere here but, for the life of me, I can't find it now 😞. The issue I'm referring to pointed at pipenv-setup as a way to handle setup.py + pipenv integration. One of the pipenv maintainers suggested that, perhaps, pipenv-setup itself could be integrated into pipenv.

However, upon attempting to use pipenv-setup I encountered a problem described in this existing issue and couldn't fix it using advice provided there. So I did what any programmer would in this situation: I wrote my own simple utility function to replace that tool.

The fix

The find_requirements utility function is supposed to simplify integration between pipenv and setup.py scripts. Instead of synchronising them manually, users could just

# file: setup.py

from setuptools import setup, find_packages
from pipenv.setuptools import find_requirements

setup(
    name="very-nice-package",
    version="0.1.0",
    author="John Doe",
    description="The Nicest Package",
    url="https://example.com/very-nice-package",
    python_requires=">=3.9, <4",
    packages=find_packages(),
    install_requires=find_requirements(),
)

This would fetch requirements from Pipfile.lock every time they run

pip install .  # or `python setup.py install`

This solution also does not rely on a separate tool one would have to install and regularly run, integrate into CI/CD, etc. Using find_requirements is simple, straightforward, and embedded with each call to setup.py.

Dear Reviewer,

This is a quick and simple implementation and it could probably benefit from a few utility functions you guys have already written that I am not aware of just yet. If you can suggest such functions or, at least, point me in their general direction, I would be happy to investigate and use them to improve this PR.

This PR does not have an associated issue because I decided that this code is simple enough and if it is rejected during review I won't be too upset about the time it took me to write it.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

find_requirements utility function is supposed to simplify integration
between `pipenv` and `setup.py` scripts. Instead of synchronizing them
manually, users could just

```python
# file: setup.py

from setuptools import setup, find_packages
from pipenv.setuptools import find_requirements

setup(
    name="very-nice-package",
    version="0.1.0",
    author="John Doe",
    description="The Nicest Package",
    url="https://example.com/very-nice-package",
    python_requires=">=3.9, <4",
    packages=find_packages(),
    install_requires=find_requirements(),
)
```

This would fetch requirements from Pipfile.lock every time they run

```bash
pip install .
```
@oz123
Copy link
Contributor

oz123 commented Apr 13, 2023

It seems like the python community has decided to move away from 'setup.py' so I think it's late to the party. Maybe we can merge it.
I need to think about it.
@matteius ?

@matteius
Copy link
Member

I disagree with adding a utility that isn't actually in use within pipenv. Also we should never add a file setuptools.py that shadows the package name setuptools.

@sharpvik
Copy link
Author

@oz123

... Python community departed from setup.py ...

I've been mostly working with Go since 2019, perhaps I missed this shift. What are people using nowadays?

@sharpvik
Copy link
Author

@matteius

I disagree with adding a utility that isn't actually in use within pipenv

I understand your reasoning and I still think that it could be useful. Of course, it is up to you guys whether you wish to include it or not, thought one small utility function put into its own package won't hurt things much.

Also we should never add a file setuptools.py that shadows the package name setuptools

That's true tho. I can rename the file, of course.

The main concern here is, I believe, the thing that @oz123 said about Python community departing from setup.py files.

@oz123
Copy link
Contributor

oz123 commented Apr 19, 2023

The community isn't departing setup.py, I am sorry for being terse and wrong.
setup.py was used to do many things when imported, and among them build the project.
It is now used, if you use the setuptools build system. But if one choose another build system,
the project directory might not have it at all.

See here:
https://godatadriven.com/blog/a-practical-guide-to-setuptools-and-pyproject-toml/

See also more on build systems for python projects:
https://packaging.python.org/en/latest/tutorials/packaging-projects/

I guess adding this to our project, would be one of the first steps of making pipenv a build system.
I imagine pipenv build subcommand building on top of this, to build wheels and source distribution.

@matteius would you support adding this if renamed?

@sharpvik
Copy link
Author

sharpvik commented Apr 22, 2023

Wow, @oz123, when I opened this issue I didn't recognise how far it could lead us. I love your idea of the pipenv build command.

Realistically, I think it would be the next logical step. For now, Pipenv is mostly a development tool for managing dependencies. For everything else you run pipenv requirements and go from there. But it doesn't have to be like that.

Pipenv reminds me of Rust's Cargo in many ways, yet cargo can build applications. If we were to introduce the build command - that's definitely a game changer. And I'd be happy to lend my helping hand if needed! 😃

@oz123
Copy link
Contributor

oz123 commented Apr 22, 2023

TBH it's been my secret desire for a long time. Although I invested much time in cleaning pipenv dependencies. That last goal is about to be reached.
Then I'll be serious about the build system.
I still want to hear about Matt's opinion.

@sharpvik
Copy link
Author

I'd like to also note that this function would be a bit more involved if we wanted to make it work with all possible types of import allowed by the setup scripts. Currently, I altered it to accept Git-based imports like so:

def find_requirements():
    def requirement(name: str, info: dict[str, str]) -> str:
        return (
            f"{name} @ git+{info['git']}@{info['ref']}#egg={name}"
            if "git" in info
            else name + info["version"]
        )

    with open("Pipfile.lock") as lock:
        requirements: dict[str, dict] = json.load(lock)["default"]
        return [requirement(name, info) for name, info in requirements.items()]

@matteius
Copy link
Member

My opinion is not to merge this function that is not in use by pipenv, and to invest more time into defining what would give pipenv package publication abilities. I think that we would not want to parse the Pipfile or Pipfile.lock for package publishing, and these are intended for managing installable requirements that are project wide. Instead I think we should adopt the similar pyproject.toml, setup.cfg (and we probably have to support setup.py install_requires as well, but it should not just pull everything from the lock file IMO).

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.

3 participants