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

bazarr: update 1.4.2 => 1.4.5 and migrate to ffmpeg 7 #6164

Merged
merged 5 commits into from
Nov 10, 2024

Conversation

smaarn
Copy link
Contributor

@smaarn smaarn commented Jun 29, 2024

Description

Update bazarr 1.4.2 => 1.4.5 and switch to ffmpeg 7

Fixes #6257

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Package update

@smaarn
Copy link
Contributor Author

smaarn commented Sep 15, 2024

Currently blocked by #6197

@mreid-tt
Copy link
Contributor

Currently blocked by #6197

Hey @smaarn, I’ve had a few recent PRs that built successfully with Python. Could you try rebasing against the current master? This might help resolve the issue you're facing, even if it doesn’t directly address the Python build problem. Let me know if this works for you.

@mreid-tt
Copy link
Contributor

Hi @smaarn, sorry to hear that the rebase didn’t work on your end—it seemed to work fine for me in #6187. Also, @th0ma7 mentioned here that many of the errors could be ignored, but I understand your situation is different since your build isn’t completing.

@smaarn smaarn changed the title update bazarr 1.4.2 => 1.4.3 update bazarr 1.4.2 => 1.4.4 Sep 18, 2024
@smaarn smaarn changed the title update bazarr 1.4.2 => 1.4.4 update bazarr 1.4.2 => 1.4.5 Oct 6, 2024
@smaarn smaarn changed the title update bazarr 1.4.2 => 1.4.5 bazarr: update 1.4.2 => 1.4.5 Oct 6, 2024
@smaarn
Copy link
Contributor Author

smaarn commented Oct 6, 2024

Hi @smaarn, sorry to hear that the rebase didn’t work on your end—it seemed to work fine for me in #6187. Also, @th0ma7 mentioned here that many of the errors could be ignored, but I understand your situation is different since your build isn’t completing.

Actually the issue seems now to be related to the below:

lrm91yfb/numpy_50629a861b5a48619313acd3869dbdad/numpy/distutils/command/config.py", line 19, in <module>
            from numpy.distutils.mingw32ccompiler import generate_manifest
          File "/tmp/pip-wheel-lrm91yfb/numpy_50629a861b5a48619313acd3869dbdad/numpy/distutils/mingw32ccompiler.py", line 27, in <module>
            from distutils.msvccompiler import get_build_version as get_build_msvc_version
        ModuleNotFoundError: No module named 'distutils.msvccompiler'

Essentially our way of installing numpy seems to be broken by latest setuptools version. (see pypa/setuptools#3505)

@hgy59
Copy link
Contributor

hgy59 commented Oct 6, 2024

Hi @smaarn, sorry to hear that the rebase didn’t work on your end—it seemed to work fine for me in #6187. Also, @th0ma7 mentioned here that many of the errors could be ignored, but I understand your situation is different since your build isn’t completing.

Actually the issue seems now to be related to the below:

lrm91yfb/numpy_50629a861b5a48619313acd3869dbdad/numpy/distutils/command/config.py", line 19, in <module>
            from numpy.distutils.mingw32ccompiler import generate_manifest
          File "/tmp/pip-wheel-lrm91yfb/numpy_50629a861b5a48619313acd3869dbdad/numpy/distutils/mingw32ccompiler.py", line 27, in <module>
            from distutils.msvccompiler import get_build_version as get_build_msvc_version
        ModuleNotFoundError: No module named 'distutils.msvccompiler'

Essentially our way of installing numpy seems to be broken by latest setuptools version. (see pypa/setuptools#3505)

I faced the same issue with numpy #6200 (comment)
Tried to use setuptools 59.2.0 as proposed by numpy. It did not work after installation of setuptools 59.2.0 into the cross env.
And using setuptools 59.2.0 in native/python311 gave an error due version conflict with setuptools-rust.
I guess we have to use setuptools 59.2.0 and compatible setuptools-rust everywhere. The cross compilation of pyhton wheels is really a mess.

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 6, 2024

Actually its a combination of things, including pip. That's why we need a per-wheel crossenv option. I intend to continue having a default using latest versions, and have the possibility, such as with older numpy, to create per wheel as needed with the set of mandatory prerequisites.

Next on my todo list now that ffmpeg is done.

@hgy59
Copy link
Contributor

hgy59 commented Oct 12, 2024

@smaarn when #6200 is merged, bazarr builds will work again, but you have to downgrade numpy to v1.24.4,

@smaarn smaarn marked this pull request as ready for review October 13, 2024 18:49
@smaarn smaarn changed the title bazarr: update 1.4.2 => 1.4.5 bazarr: update 1.4.2 => 1.4.5 and migrate to ffmpeg 7 Oct 13, 2024
@smaarn
Copy link
Contributor Author

smaarn commented Oct 13, 2024

@smaarn when #6200 is merged, bazarr builds will work again, but you have to downgrade numpy to v1.24.4,

@hgy59 I managed to have everything build successfully after rebase. Why were you expecting me to need to downgrade numpy ?

@hgy59
Copy link
Contributor

hgy59 commented Oct 13, 2024

@smaarn in #6200 I already downgraded numpy for DSM 7 from 1.25.1 to 1.24.4 (requirements-crossenv-numpy.txt)

@hgy59
Copy link
Contributor

hgy59 commented Nov 1, 2024

@smaarn in preparation for #6255 can you please update the spk/bazarr/Makefile and replace DEPENDS = cross/$(SPK_NAME) by DEPENDS = cross/bazarr?

For #6255 it will be mandatory that none of the DEPENDS* definitions use makefile variables anymore.

BTW: update of bazarr LGTM

@smaarn
Copy link
Contributor Author

smaarn commented Nov 1, 2024

@smaarn in preparation for #6255 can you please update the spk/bazarr/Makefile and replace DEPENDS = cross/$(SPK_NAME) by DEPENDS = cross/bazarr?

For #6255 it will be mandatory that none of the DEPENDS* definitions use makefile variables anymore.

BTW: update of bazarr LGTM

@hgy59 done in 50e80d1

Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

LGTM

IMHO we can create the qoriq package later, when #6297 is fixed

@mreid-tt
Copy link
Contributor

@smaarn, I've done a re-base for you which fixes the qoriq build. It should be good to merge now.

@hgy59
Copy link
Contributor

hgy59 commented Nov 10, 2024

@mreid-tt please never rebase a foreign branch!

It is always enough to merge the upstream master into such a branch.
Otherwise it is very hard for the owners to proceed with their unpushed changes.

@mreid-tt mreid-tt merged commit ba6416f into SynoCommunity:master Nov 10, 2024
15 checks passed
@mreid-tt mreid-tt added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/needs-review labels Nov 10, 2024
@smaarn smaarn deleted the bazarr/update-to-1.4.3 branch November 10, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
4 participants