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

Bump python-vlc to 3.0.18122 #94739

Merged
merged 1 commit into from Oct 7, 2023
Merged

Conversation

atudor2
Copy link
Contributor

@atudor2 atudor2 commented Jun 16, 2023

Proposed change

Bump version of python-vlc to 3.0.18122 to resolve #94720
Its a bit difficult for me to fully test the vlc integration locally, but tests pass and integration loads without error.

Changes: https://github.com/oaubert/python-vlc/commits/master/generated/3.0/vlc.py

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@elupus
Copy link
Contributor

elupus commented Jun 18, 2023

Please add a link to changes in the new release of vlc plugin in the description.

@elupus elupus reopened this Jun 18, 2023
@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jun 21, 2023
@frenck
Copy link
Member

frenck commented Jun 27, 2023

I've marked this PR, as changes are requested that need to be processed.
Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

@frenck frenck marked this pull request as draft June 27, 2023 10:32
@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Jun 27, 2023
@atudor2
Copy link
Contributor Author

atudor2 commented Jun 27, 2023

Apologies this fell off my todo list
These are generated bindings for libvlc and the news site hasn't been updated in 4 years.
Will the commit history on the generated file suffice?

@atudor2 atudor2 marked this pull request as ready for review June 27, 2023 15:44
@frenck
Copy link
Member

frenck commented Jul 25, 2023

@atudor2 How did you test this?

@atudor2
Copy link
Contributor Author

atudor2 commented Jul 25, 2023

@frenck unfortunately it's a bit difficult for me to fully test this integration locally, but I re-ran the unit tests and confirmed that the integration loads without error following the version bump

@frenck
Copy link
Member

frenck commented Jul 25, 2023

unfortunately it's a bit difficult for me to fully test this integration locally,

I will not be able to accept this PR without it being properly tested.

but I re-ran the unit tests and confirmed that the integration loads without error following the version bump

There are no unit tests for this integration....

@atudor2
Copy link
Contributor Author

atudor2 commented Jul 25, 2023

👍 I'll see if I can setup a venv instance and do further tests in the next few days.
I'll move this PR back to draft in the meantime

@atudor2 atudor2 marked this pull request as draft July 25, 2023 19:50
@ErikApption
Copy link

ErikApption commented Jul 25, 2023

I applied this PR and changed homeassistant/components/vlc/manifest.json with the new version of python-vlc and I can confirm that the VLC integration works as expected. I am happy to run other tests, but not sure how a unit test would look like. This starts the VLC process and doesn't throw an error message on startup. This is caused by a Python 3.11 breaking change - without this PR, the VLC integration is completely broken.

@atudor2
Copy link
Contributor Author

atudor2 commented Jul 30, 2023

Thanks @ErikApption for the feedback
@frenck would this be sufficient for testing confirmation? Haven't had a chance yet to sit down and setup a core install yet

@ErikApption
Copy link

Thanks @ErikApption for the feedback @frenck would this be sufficient for testing confirmation? Haven't had a chance yet to sit down and setup a core install yet

Tested with 2023.8 and I can confirm this PR is necessary for the integration to work in 2023.8.

@atudor2
Copy link
Contributor Author

atudor2 commented Aug 14, 2023

I setup a core install and managed to test this my side as well playing, pausing and stopping an mp3 file - will reopen this PR for review

@ErikApption
Copy link

Anyway way to help on this one - I don't understand the risk of merging this one, as of today the VLC integration is just broken

@letshin
Copy link

letshin commented Oct 5, 2023

I'm having the same issue too and I was using vlc-telnet without issues earlier.

Copy link

@averater averater left a comment

Choose a reason for hiding this comment

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

Very easy fix. Please merge!

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Oct 7, 2023
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @atudor2 👍

../Frenck

@frenck frenck merged commit b60401b into home-assistant:dev Oct 7, 2023
34 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed dependency integration: vlc Quality Scale: No score small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VLC broken after 2023.6 update (cannot import name 'getargspec' from 'inspect')
6 participants