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 PySide6 to rosdep keys #42353

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add PySide6 to rosdep keys #42353

wants to merge 7 commits into from

Conversation

samyarsadat
Copy link

Please add the following dependency to the rosdep database.

Package name:

PySide6

Package Upstream Source:

https://github.com/qtproject/pyside-pyside-setup

Purpose of using this:

This package is a Python wrapper for Qt 6. It allows one to create Qt applications using Python.

Links to Distribution Packages

@samyarsadat samyarsadat requested a review from a team as a code owner August 8, 2024 16:16
@github-actions github-actions bot added the rosdep Issue/PR is for a rosdep key label Aug 8, 2024
rosdep/python.yaml Outdated Show resolved Hide resolved
@marcoag
Copy link
Contributor

marcoag commented Aug 12, 2024

Standard pip disclaimer: ROS packages that depend on pip keys cannot be released into a ROS distribution. They can only be depended on by from-source builds. Because of this, system packages are highly preferred to pip packages.

audrow
audrow previously approved these changes Aug 16, 2024
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This one ends up being tricky.

We know that system packages for pyside6 are coming; they are already available for Ubuntu Oracular and Debian Sid.

However, as a matter of course we don't usually add in keys for pre-release distributions.

There are 2 ways we could go here:

  1. Take in this python3-pyside6-pip key for now, and later add in a python3-pyside6 key.
  2. Right now go for a python3-pyside6 key, and fill it out with pip for everything except for very new Ubuntu and Debian distributions.

Personally, I think we should go for 2, but I can be convinced otherwise.

@mjcarroll @alsora Any thoughts here?

@clalancette clalancette added the changes requested Maintainers have asked for changes to the pull request label Aug 20, 2024
@samyarsadat
Copy link
Author

I also think number 2 is a better option, but there is one small issue with it.
As with PySide2, the PySide6 binary packages are split up into different modules (ex. python3-pyside6.qtqml, python3-pyside6.qtgui, etc.), so keys for all of those would have to be added as well (at least for the latest Debian and Ubuntu distros that are going to use system packages).

@clalancette
Copy link
Contributor

As with PySide2, the PySide6 binary packages are split up into different modules (ex. python3-pyside6.qtqml, python3-pyside6.qtgui, etc.), so keys for all of those would have to be added as well (at least for the latest Debian and Ubuntu distros that are going to use system packages).

Yeah, agreed. However, we can do that piecemeal. What particular parts of PySide6 are you interested in?

@alsora
Copy link
Contributor

alsora commented Aug 21, 2024

I agree with option 2.
It also seems slightly more consistent with the rest of the file, where the -pip suffix is used only for those dependencies that always come from pip, while no suffix is used for dependencies where we use a mixture of pip and system packages.

@samyarsadat
Copy link
Author

As with PySide2, the PySide6 binary packages are split up into different modules (ex. python3-pyside6.qtqml, python3-pyside6.qtgui, etc.), so keys for all of those would have to be added as well (at least for the latest Debian and Ubuntu distros that are going to use system packages).

Yeah, agreed. However, we can do that piecemeal. What particular parts of PySide6 are you interested in?

That works. I currrently need qtcore, qtgui, qtconcurrent, qtqml, and qtwidgets.

Should there be one key named python3-pyside6 that installs PySide6 thourgh pip on distros with no system packages and libpyside6-dev, python3-pyside6.qtcore, python3-pyside6.qtgui, python3-pyside6.qtconcurrent, python3-pyside6.qtqml, and python3-pyside6.qtwidgets on distros with system packages, or should there be individual keys for all of the different modules?

@clalancette
Copy link
Contributor

or should there be individual keys for all of the different modules?

I would start with individual keys. That gives packagers maximum control. If it turns out that there is really a need for a "consolidated" key, we can always add that in later.

@samyarsadat
Copy link
Author

I've updated the python3-pyside6 key and added all the new keys for the different modules.
I previously said that I needed qtcore, qtgui, qtconcurrent, qtqml, and qtwidgets, but I've added a few more:
qtasyncio, qtcharts, qtdatavisualization, qttest.

Copy link

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

@github-actions github-actions bot added the stale Issue/PR hasn't been active in a while and may be closed. label Sep 12, 2024
@github-actions github-actions bot removed the stale Issue/PR hasn't been active in a while and may be closed. label Sep 13, 2024
@sloretz sloretz dismissed stale reviews from audrow and marcoag September 19, 2024 16:54

Changes since review

@sloretz sloretz removed the changes requested Maintainers have asked for changes to the pull request label Sep 19, 2024
bullseye:
pip:
packages: [PySide6]
sid: [libpyside6-dev]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sid: [libpyside6-dev]
trixie: [libpyside6-dev]
sid: [libpyside6-dev]

Add the current Debian Testing distribution as well. I would even consider dropping the sid definition altogether in favor of just trixie but don't mind it being there.

It's worth making the change for each of the definitions below as well.

Copy link
Member

@nuclearsandwich nuclearsandwich Sep 19, 2024

Choose a reason for hiding this comment

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

Done in 590b6ae

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this made CI unhappy

    ERR: list out of alphabetical order line 8526.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8551.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8557.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8563.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8569.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8575.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8581.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8587.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8593.  'sid' should come before 'trixie'
    ERR: list out of alphabetical order line 8599.  'sid' should come before 'trixie'

@nuclearsandwich
Copy link
Member

@sloretz I modified the PR to include trixie definitions, would you please give it an updated review with my changes.

I'd intentionally tried to keep these in release order rather than
lexicographical order but I forgot that the checker would dislike this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rosdep Issue/PR is for a rosdep key
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants