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

Use pdm as a helper and a build backend #140

Closed
wants to merge 8 commits into from
Closed

Conversation

getzze
Copy link
Contributor

@getzze getzze commented Oct 29, 2024

fixes #127, #128
Supersedes #138

  • Use pyproject.toml with pdm backend.
  • Use src (avoid some errors while running tests) and scripts folders.
  • Bump minimal python version to 3.9 (although it should work fine on py3.8, but it just reached EOL). Support python 3.13.
  • Build wheel with bundled libmediainfo for MacOS arm64 and for Linux (x886_64 and arm64).
  • Fix linting and type errors in source and tests.

pdm (like most "new" python build backend like hatch, poetry, flit) do not support C-extension yet. But for this package, building the wheel for different platforms only means downloading the correct flavor of the library. Hence a universal wheel is built and is later tagged with the correct platform tag using wheel.
However it's very simple to create scripts with pdm, which can be used to simplify the download process (future PR).

I don't have an appveyor account so I cannot run the tests.

Future PRs:

  • use pdm scripts to download the libraries.
  • switch from black-isort-flake8-pylint to ruff
  • use pre-commit hook.

Copy link
Owner

@sbraz sbraz left a comment

Choose a reason for hiding this comment

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

Wow thanks a lot, that is a great PR! pbr does seem to make wheel generation much easier!
I think I've asked basically all the questions I needed.
Once I'm OK with everything, do you mind if I reword some commits to add more details (e.g. why support for older Pythons was dropped, why method order was changed, etc.)?

appveyor.yml Outdated Show resolved Hide resolved
appveyor.yml Show resolved Hide resolved
pdm build --no-sdist
wheel tags --remove --platform-tag=manylinux_2_34-x86_64 dist/*-py3-none-any.whl
# wheel arm64
mv -f arm64/lib/libmediainfo.so.0.0.0 pymediainfo/libmediainfo.so.0
Copy link
Owner

Choose a reason for hiding this comment

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

If upstream changes the version, the name of the actual file but .so.0 will still be a symlink, so this would be more future-proof?

Suggested change
mv -f arm64/lib/libmediainfo.so.0.0.0 pymediainfo/libmediainfo.so.0
cp arm64/lib/libmediainfo.so.0 pymediainfo/libmediainfo.so.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean, the .so.0.0.0 is the real file, the .so.0 is just a symlink to it. Isn't it better to copy the real file?

Copy link
Owner

@sbraz sbraz Oct 29, 2024

Choose a reason for hiding this comment

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

In the code, the default is to use library_paths = ("libmediainfo.so.0",). This means that this library (pymediainfo) expects it to exist. If it doesn't, there's something wrong and I'll have to update the code, not only the CI.
However, nothing guarantees that the real file is named libmediainfo.so.0.0.0. That's why I thought it would be better to copy libmediainfo.so.0 (cp resolves the link to copy the actual file).
Please let me know if this is clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but it's renamed to .so.0 (mv moves and renames files). It's a common practice to have the real lib file with full version and symlink to .so.0 and .so. Therefore I doubt mediainfo will change the naming scheme, but as you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I know your command renames the file to what is expected.
All I'm saying is that there is no guarantee that upstream won't change the filename later from libmediainfo.so.0.0.0 to something. On the other hand, since libmediainfo.so.0 is the SONAME, I think it is less likely for them to rename libmediainfo.so.0 without a major reason.

objdump -p libmediainfo.so.0 | grep SONAME
  SONAME               libmediainfo.so.0

@JeromeMartinez please let me know if my idea of copying the .so.0.0 by resolving the symlink from .so.0 makes more sense than directly copying the .so.0.0. Also, do you have an opinion on us bundling the AWS Lambda library into the Python wheels?

src/pymediainfo/__init__.py Show resolved Hide resolved
src/pymediainfo/__init__.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/pymediainfo/__init__.py Show resolved Hide resolved
use exact filename

Co-authored-by: Louis Sautier <[email protected]>
@getzze
Copy link
Contributor Author

getzze commented Oct 30, 2024

Please modify this PR and its commits as much as you want!

@Tohrusky
Copy link

Considering migrating to github actions? I think it's more simple for CI/CD

@sbraz
Copy link
Owner

sbraz commented Oct 30, 2024

Considering migrating to github actions? I think it's more simple for CI/CD

I considered it at some point but I like the idea of having CI/CD independent from the code. If I ever want to move this project out of GitHub, I can still keep using AppVeyor.
Also making the CI work took quite some time, then Travis basically stopped being free so I had to migrate to AppVeyor… I'm not sure I want to migrate a second time unless it's really helpful.
Let's first make these wheels work. Then, we can decide what we want to do with the CI/CD.

@sbraz
Copy link
Owner

sbraz commented Nov 17, 2024

@getzze I'm reworking your commits right now. I'm removing ruff/coverage/typos from setup.py (basically, everything not used by the CI/CD right now). I'll do it in my own branch so it won't get lost for future PRs.

sbraz pushed a commit that referenced this pull request Nov 17, 2024
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x886_64 and arm64).

Fixes: #127
Fixes: #128
Fixes: #140
setup.cfg Show resolved Hide resolved
sbraz pushed a commit that referenced this pull request Nov 17, 2024
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x86_64 and arm64).
Instead of installing MediaInfo via apt to run tests, use the AWS Lambda
library file that we bundle into Linux wheels, this simplifies the CI
code.

Fixes: #127
Fixes: #128
Fixes: #140
sbraz pushed a commit that referenced this pull request Nov 17, 2024
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x86_64 and arm64).
Instead of installing MediaInfo via apt to run tests, use the AWS Lambda
library file that we bundle into Linux wheels, this simplifies the CI
code.

Fixes: #127
Fixes: #128
Fixes: #140
@@ -0,0 +1,199 @@
# https://packaging.python.org/en/latest/specifications/pyproject-toml/
[build-system]
requires = ["pdm-backend", "wheel>=0.42"]
Copy link
Owner

Choose a reason for hiding this comment

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

Should wheel really be here? wheel is not required for the sdist, right? I'm afraid this is going to cause issues to Linux distributions that package pymediainfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed to build the wheels but not the sdist. Why do you think it will be an issue to package it with Linux distribution? python-wheel is part of the new build workflow, cut off from setuptools, I am sure it's packaged for all the distros.

Copy link
Owner

Choose a reason for hiding this comment

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

We at Gentoo don't like declaring dependencies when they're not actually needed. Isn't there a better way to declare it as a dependency only for the wheels? Ping @mgorny.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fair enough.
The dependencies defined in build-system are used when you do pip install .
In this case you don't need to build a wheel with the libs bundled. So you can remove wheel from these deps.
What can be done then, is to put the wheel dep in a pdm script that builds the bundled wheels (I'm preparing this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@getzze
Copy link
Contributor Author

getzze commented Feb 7, 2025

@sbraz Hey, any update for this PR? Let me know if you need help to finish it.

@sbraz
Copy link
Owner

sbraz commented Feb 7, 2025

Hi @getzze, I will try to finish this over the weekend. I'm very sorry for how long it's taking.

sbraz pushed a commit that referenced this pull request Feb 9, 2025
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x86_64 and arm64).
Instead of installing MediaInfo via apt to run tests, use the AWS Lambda
library file that we bundle into Linux wheels, this simplifies the CI
code.

Fixes: #127
Fixes: #128
Fixes: #140
sbraz pushed a commit that referenced this pull request Feb 10, 2025
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x86_64 and arm64).
Instead of installing MediaInfo via apt to run tests, use the AWS Lambda
library file that we bundle into Linux wheels, this simplifies the CI
code.

Fixes: #127
Fixes: #128
Fixes: #140
sbraz pushed a commit that referenced this pull request Feb 11, 2025
Build wheel with bundled libmediainfo for MacOS arm64 and for Linux
(x86_64 and arm64).
Instead of installing MediaInfo via apt to run tests, use the AWS Lambda
library file that we bundle into Linux wheels, this simplifies the CI
code.

Fixes: #127
Fixes: #128
Fixes: #140
@sbraz sbraz closed this in bf780d1 Feb 11, 2025
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.

Add support for macOS ARM
3 participants