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

Update build to use exclusively pyproject.toml #5836

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

kalebmckale
Copy link
Contributor

@kalebmckale kalebmckale commented Aug 19, 2023

Thank you for contributing to Pipenv!

The goal: modernize build

  • consolidates all of setuptools metadata and configuration as well as coverage configuration within pyproject.toml.
  • removes deprecated setup.cfg and setup.py.
  • updates run-tests.sh to use pyproject.toml instead of setup.cfg for coverage configuration.

The checklist

  • Associated issue: Update build to use exclusively pyproject.toml #5837
  • 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 #.

Resolves #5837

pyproject.toml Outdated Show resolved Hide resolved
@kalebmckale
Copy link
Contributor Author

kalebmckale commented Aug 20, 2023

Never mind.

@kalebmckale
Copy link
Contributor Author

Seems like everything is working except the versioning. I expect this is because setuptools_scm cannot see the git tag. I figured this might come up since there’s already an in-place script that handles updating version, but thought it might still work. Nevertheless, using setuptools_scm isn’t a requirement to make this work and really unnecessary since there seems to be an existing solution that doesn’t require human intervention. So, if it ain’t broke…. I’ll make the changes to pull the version from the __version__.py file instead.

@kalebmckale kalebmckale marked this pull request as ready for review August 20, 2023 02:24
news/5837.behavior.rst Outdated Show resolved Hide resolved
@oz123
Copy link
Contributor

oz123 commented Aug 20, 2023

@kalebmckale please pull the changes from the master branch. This will fix the ruff errors.

Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

It's not a behaviour change. Please add this to feature.
(e.g. use XXXX.feature.rst).

pyproject.toml Outdated Show resolved Hide resolved
@kalebmckale
Copy link
Contributor Author

Please rebase.

Sorry I couldn't get to it until tonight.

@kalebmckale kalebmckale changed the title Draft: Update build to use exclusively pyproject.toml Update build to use exclusively pyproject.toml Aug 23, 2023
@matteius matteius requested a review from oz123 August 25, 2023 20:17
pyproject.toml Show resolved Hide resolved
@matteius
Copy link
Member

Hey @kalebmckale if you want to join the unofficial Python Developers slack group https://pyslackers.com/web we can chat more in there -- I am thinking we could bounce ideas off each-other about the currently broken editable/local file installs.

@kalebmckale
Copy link
Contributor Author

kalebmckale commented Aug 26, 2023

Ok @matteius... um never used slack so how do I find you on there? 🙂

Update: I might have found you.

@kalebmckale kalebmckale force-pushed the draft-update-build branch 3 times, most recently from 08a8d77 to 00de64d Compare August 27, 2023 19:18
@kalebmckale
Copy link
Contributor Author

Rebased on v2023.8.26.

@kalebmckale
Copy link
Contributor Author

Added a new commit to test if adding __version__.py to .gitignore will break the release process. I suspect this will not be tested until after the merge has taken place and release.py is called since I could find no reference in the repo files themselves to call this script (and assume it’s a setting in the administrative CI/CD process).

@kalebmckale kalebmckale requested a review from oz123 August 27, 2023 19:48
@oz123
Copy link
Contributor

oz123 commented Aug 27, 2023

@kalebmckale I am sorry. I thought we need to exclude __version__ as that is what one should do with https://github.com/pypa/setuptools_scm. However, you are using https://setuptools.pypa.io/en/latest/build_meta.html.
build_meta does not care about storing the version in git. Once again, than you for taking the time for contributing and apologies for the confusion (and extra work required from you).

.gitignore Outdated Show resolved Hide resolved
@kalebmckale
Copy link
Contributor Author

Just noticed this in release.py

@invoke.task
def build_dists(ctx):
    drop_dist_dirs(ctx)
    py_version = ".".join(str(v) for v in sys.version_info[:2])
    env = {"PIPENV_PYTHON": py_version}
    with ctx.cd(ROOT.as_posix()), temp_environ():
        executable = ctx.run(
            "python -c 'import sys; print(sys.executable)'", hide=True
        ).stdout.strip()
        log("Building sdist using %s ...." % executable)
        os.environ["PIPENV_PYTHON"] = py_version
        ctx.run("pipenv install --dev", env=env)
        ctx.run("pipenv run pip install -e . --upgrade --upgrade-strategy=eager", env=env)
        log("Building wheel using python %s ...." % py_version)
        ctx.run("pipenv run python setup.py sdist bdist_wheel", env=env)

I know during testing the package is being built and installed, or it at least appears to do so, but here it looks to still use setup.py for build.

@kalebmckale
Copy link
Contributor Author

Found in Makefile, too.

.PHONY: build
build: install-virtualenvs.stamp install.stamp
	PIPENV_PYTHON=3.7 pipenv run python setup.py sdist bdist_wheel

@kalebmckale
Copy link
Contributor Author

And pypi_upload.yml

    - name: Build wheels
      run: |
        python -m pipenv run python setup.py sdist bdist_wheel

@matteius
Copy link
Member

That is what I was afraid of -- if we merge this, we will break the pypi upload of the built wheel capabilities, I suspect.

@kalebmckale
Copy link
Contributor Author

kalebmckale commented Aug 28, 2023

Made changes, but I don't have ability to lock Pipfile on my iPad. So, if approved, will need a maintainer to make this update.

UPDATE: Apparently will need this for testing, too.

@cclauss
Copy link
Contributor

cclauss commented Aug 31, 2023

Successfully created virtual environment!
Virtualenv location: /Users/runner/.local/share/virtualenvs/pipenv-XC1sj3ZQ
Your Pipfile.lock (4d9c54) is out of date. Expected: (60755f).
Usage: pipenv install [OPTIONS] [PACKAGES]...

What would happen if you did git rm Pipfile.lock? Would the GitHub Action test process recreate that file?

@matteius
Copy link
Member

I don't want to remove the Pipfile.lock from the checked in code though-- there will be a path forwards on this, even if it means merging into a different pipenv branch so I can run the lock phase.

@kalebmckale
Copy link
Contributor Author

I was able to lock on my iPad using Python 11, then I realized that would probably not be good since the minimum Python version is 3.7. I’ve been working for days to get other options. I can get Python 3.9 and looking to see if I can get Python 3.7. If there’s another solution sooner, we should go with that. However, I may be able to eventually get it done.

@kalebmckale
Copy link
Contributor Author

@matteius Another option is to include an updated locked Pipfile in another merge/release. Adding the two dev packages wouldn’t interfere with other changes.

@cclauss
Copy link
Contributor

cclauss commented Aug 31, 2023

If the minimum Python version is 3.7 then Python 3.11 is acceptable.

@cclauss
Copy link
Contributor

cclauss commented Sep 1, 2023

- consolidates all of `setuptools` metadata and configuration as well
as `coverage` configuration within `pyproject.toml`.
- removes deprecated `setup.cfg` and `setup.py`.
- updates `run-tests.sh` to use 	`pyproject.toml` instead of
`setup.cfg` for `coverage` configuration.
Found left-over remnants of `setup.py` install and updated with current
build method using `build` package.
pyproject.toml Show resolved Hide resolved
[project]
name = "pipenv"
authors = [
{name = "Pipenv maintainer team", email = "[email protected]"},
Copy link
Contributor

Choose a reason for hiding this comment

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

The email is wrong. Please correct the email address. @matteius should our email adresses be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I've been working on this PR for two weeks now and rebased multiple times. Someone else is welcome to take this on, but I've surpassed the amount of work I'm willing to put in at this time.

Copy link
Member

Choose a reason for hiding this comment

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

We will update the email address outside this PR -- thanks for your effort on this @kalebmckale

@matteius matteius merged commit 76edf74 into pypa:main Sep 5, 2023
19 checks passed
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.

Update build to use exclusively pyproject.toml
4 participants