-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Conversation
After some reflections I've had a second thought.
Going to revise my solution accordingly. |
419d841
to
9e16f11
Compare
It seems to be working a little bit better now. Things to do:
Any suggestions/comments/guidelines are still welcome. |
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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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...
Bottom line:
|
After a while, I hack the Wheel metadata to get information if package is pure or not. |
It reached "works for me" stage. |
@@ -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) |
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?] |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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?
a9a1434
to
43fefae
Compare
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 |
I don't understand what this does? It still requires a legacy And if you have a source python project that has the option to generate a legeacy I am a bit confused what this change is supposed to do. What is the intention of this change? |
No, it doesn't. There is an option to use |
Thanks for the clarification. I'll need to read through the patch again then. 😁 |
Although in this MR I made attempt to solve the issue via |
That's excellent. 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 |
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. |
Quick feedback from my initial testing of this patch: Testing with
My main issue that I'm looking into at the moment is the boot-strapping of the PEP 517 build environments for I think the main concern so far is the new dependency on the But a lot of wheels will now write 2.3 Metadata file formats. Therefore, injecting or vendoring an up-to-date version of |
@cwegener could you please elaborate, what is your issue with |
I think it's actually a different issue that is unrelated to the PEP-517 build support. The issue is that all dependencies are being built from source due to a change introduced here: 77e0cf7 The The dependencies will be thrown away at the end. They don't end up in the released package that fpm creates. So there is really no benefit in building them from source. Nevertheless, I still have not managed to build an |
Just checked - built seamlessly. Could you please try to build it with more verbose output and share the result? (you may want to adjust |
I had another look. And I think I figured out where the issue with this approach is. Example package:
This will try to immediately build a wheel from the This is a References: |
Could you please elaborate, do you able to offer different approach? May be you know the way to download and use pre-build binary packages, for all possible supported OS/architectures? |
@amdei This branch worked for me on ubuntu 24.04! On ubuntu 22.04, ubuntu 20.04, rockylinux 8 and rockylinux 9, I had to both upgrade the distro-provided pip as well as upgrade the above modules before the creation of a .deb from python/pip would work. So, I conclude that the core of your patch works great, but there may be some dependency auto-foo that is lacking? Thank you for submitting this MR! |
TL;DR:
It works! (With some limitations at the moment though, as this is work in progress currently)
Limitations:
Temporary limitations (TODOs):
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