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

Add support for converting pyproject.toml-based Python3 packages. #1982

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

amdei
Copy link

@amdei amdei commented Jan 3, 2023

TL;DR:
It works! (With some limitations at the moment though, as this is work in progress currently)

git clone ....
cd fpm
python3 -m pip install importlib_metadata
ruby bin/fpm --verbose --log debug --debug --debug-workspace -s python -t deb --python-install-lib=/usr/local/lib/python3.9/dist-packages/ --python-package-name-prefix python3 --python-bin=python3  typing_extensions

Limitations:

  1. Does not support python2 in any way (obviously)

Temporary limitations (TODOs):

  1. Do not support automatic dependencies extraction to the moment
  2. Do not support license extraction to the moment
  3. Checked only on Debian 11 amd64 (arm64 to go) so far
  4. Not sure about applicability to 'native' packages
  5. A lot of @todo's in code
  6. Code quality is poor
  7. Code-style is poor

I had no previous experience with ruby, so any suggestions/comments/guidelines are welcome.

@ObjatieGroba - could you please check Python part of code, and give your suggestions on how to improve it?

Fixes #1780
Fixes #1873
Fixes #1860
Fixes magma/magma#13847

@amdei
Copy link
Author

amdei commented Jan 6, 2023

After some reflections I've had a second thought.

  1. Standard importlib.metadata is better that backported importlib_metadata
  2. importlib.metadata is still not good enough to obtain all required metadata of not-yet-installed python package
  3. I don't want to build wheel out of downloaded python package
  4. But seems there is no solution without building wheel out of python package prior to converting it to some other format
  5. It is possible and it is easy to get ALL required metadata via wheel API from pkginfo package

Going to revise my solution accordingly.
Stay tuned.

@amdei
Copy link
Author

amdei commented Feb 12, 2023

It seems to be working a little bit better now.
At least in terms of metadata retrieval.

Things to do:

  • 1. Apply proper arch attribute to generated deb-package. At the moment all toml-based packages has Architecture: all;
  • 2. Instruct wheel to not download dependencies for build package wheel;
  • 3. For some packages fpm now silently dies with zero exit code. Need to get it work again;
  • 4. Deal with packages dependencies. Not easy at the first glance.
  • 5. Speed-up execution on processing toml packages. Now it 3 times slower that setup.py-based;
  • 6. Perform code de-duplication. As now it contains significant amount of redundant fragments.
  • 7. Cleanup code.
  • 8. File some issues to pip, wheel, importlib, etc.
  • 9. Obey --[no-]python-obey-requirements-txt fpm's option

Any suggestions/comments/guidelines are still welcome.
As well as package samples, where things go wrong.

@jordansissel
Copy link
Owner

Thanks for your work on this! I’ll take a look as soon as I can :)

return deps

def get_home_url(self, project_urls):
res = dict([i.strip() for i in x.split(',')] for x in project_urls)
Copy link

Choose a reason for hiding this comment

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

Theren is no reason to generate dict.
Use next(filter) instead of dict.

return next((x.split(',', maxsplits=1)[1].strip() for x in project_urls if x.lstrip().startswith('Home')), None)

Copy link
Author

Choose a reason for hiding this comment

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

It appears that different packages put home URL into different keys.
For example Home vs. Homepage.

So let's stick to a current approach, if there is no more efficient solutions...

@amdei
Copy link
Author

amdei commented Feb 14, 2023

Bottom line:

  1. Please update python wheel to latest version before trying anything: python3 -m pip install -U wheel - there was a bug, affected fpm, fixed only very recently.
  2. It is slow. Very slow. Significantly slower that setup.py-based packaging;
  3. Wasn't able to made it with architecture - it looks like there is simply no way to get arch neither from *.whl file nor from sdist via any API. But it contains in *.whl. But I'm not brave enough to parse it in that way (yet).
  4. There is no alternatives found for setup.py's --install-lib, --install-data, --install-scripts and build_scripts --executable. So all fpm's --python-install-* options will not work (and currently silently ignored).
  5. Staging with pip is broken. For example see here: Unexpected --target behavior with/without --upgrade pypa/pip#8799 Or it is intended for something else.
  6. Documentation for pip install is rather short: https://docs.python.org/3/installing/index.html#installing-index Significantly shorter than one would expect.

@amdei
Copy link
Author

amdei commented Apr 8, 2023

After a while, I hack the Wheel metadata to get information if package is pure or not.
https://peps.python.org/pep-0491/
As it seems to be impossible (at least yet) via pkginfo.Wheel API: https://bugs.launchpad.net/pkginfo/+bug/2015657

@amdei amdei marked this pull request as ready for review April 10, 2023 00:29
@amdei
Copy link
Author

amdei commented Apr 10, 2023

It reached "works for me" stage.
Let's collect feedback before starting to polish things..

@@ -83,14 +83,17 @@ def run(self):

if self.load_requirements_txt:
requirement = open(self.requirements_txt).readlines()
# print('REQ-SETUP-PY-REQ-TXT:', requirement, file=sys.stderr)

Choose a reason for hiding this comment

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

unnecessary lines

if !File.exist?(setup_py)
logger.error("Could not find 'setup.py'", :path => setup_py)
raise "Unable to find python package; tried #{setup_py}"
# @todo Swap it! - TOML priority is for debugging only!

Choose a reason for hiding this comment

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

It is time to swap

logger.debug("Do job with pyproject.toml")
wheel_path = build_py_wheel(setup_py)
load_package_info_wheel(setup_py, wheel_path)
# install_to_staging_toml(setup_py)

Choose a reason for hiding this comment

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

unnecessary comment

"#{Shellwords.escape(toml_metadata_code)}"

# @todo FIXME!
# if attributes[:python_obey_requirements_txt?]

Choose a reason for hiding this comment

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

Doesn't this break backward compatibility?

::Dir.chdir(project_dir) do
flags = [ "--root", staging_path ]

# if !attributes[:python_install_lib].nil?

Choose a reason for hiding this comment

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

Lot's of commented code. Is it necessary?

@JordanStopford
Copy link

Amy update on this? A lot of packages no longer ship setup.py files so if we need to use fpm then we're stuck on old versions

@cwegener
Copy link
Contributor

cwegener commented Apr 9, 2024

I don't understand what this does? It still requires a legacy setup.py file to be generated by the python project that is supposed to be converted. However, pretty much all PEP-517 build backends disable the support for generating setup.py legacy files by default, so they will not be able to be used by fpm. (some backends don't even offer an option at all to generate legacy setup.py files)

And if you have a source python project that has the option to generate a legeacy setup.py enabled, then such a project can already be used as a source in fpm without this change.

I am a bit confused what this change is supposed to do.

What is the intention of this change?

@amdei
Copy link
Author

amdei commented Apr 9, 2024

It still requires a legacy setup.py file

No, it doesn't.

There is an option to use setup.py file, if it presented.
But if there is pyproject.toml file - it will be used instead.

@cwegener
Copy link
Contributor

cwegener commented Apr 9, 2024

It still requires a legacy setup.py file

No, it doesn't.

There is an option to use setup.py file, if it presented. But if there is pyproject.toml file - it will be used instead.

Thanks for the clarification. I'll need to read through the patch again then. 😁

@amdei
Copy link
Author

amdei commented Apr 17, 2024

Although in this MR I made attempt to solve the issue via pip wheel, not via build module from PyPA, as suggested in #2040.

@cwegener
Copy link
Contributor

Although in this MR I made attempt to solve the issue via pip wheel, not via build module from PyPA, as suggested in #2040.

That's excellent. pip wheel will actually just invoke the Python build module which takes care of performing the right build process (legacy setup.py vs new PEP-517 build backends)

I will need to test out your PR and report the results.

Specifically, I will need to test with this PEP-517 project ... https://github.com/openai/openai-python

@Stealthii Stealthii mentioned this pull request Apr 24, 2024
@gmabey
Copy link

gmabey commented May 8, 2024

Just an enthusiastic vote for more eyeballs on this merge request ... I would love ❤️ to see the project catch up to this shift in the python community.

@cwegener
Copy link
Contributor

I will need to test out your PR and report the results.

Quick feedback from my initial testing of this patch:

Testing with

  • openai package from PyPI (which uses hatchling as the PEP 517 build backend)
  • Testing on Ubuntu 22.04 with built-in Python3.10

My main issue that I'm looking into at the moment is the boot-strapping of the PEP 517 build environments for pip wheel (and to a lesser degree pip download ... which is a separate story).

I think the main concern so far is the new dependency on the pkginfo Python library. E.g. on Ubuntu 22.04, the version of python3-pkginfo is a bit older and does not yet have support for version 2.3 Metadata file formats.

But a lot of wheels will now write 2.3 Metadata file formats.

Therefore, injecting or vendoring an up-to-date version of pkginfo is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants