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

Fix regression introduced in #21 #28

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

ahcorde
Copy link

@ahcorde ahcorde commented Jan 27, 2021

Fix regression introduced in #21. Related with this CI failure https://ci.ros2.org/job/ci_linux/13508/testReport/junit/(root)/rqt_reconfigure/test_test_text_filter/

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added the bug label Jan 27, 2021
@ahcorde ahcorde self-assigned this Jan 27, 2021
from setuptools import setup

package_name = 'rqt_console'

setup(
name=package_name,
version='1.1.1',
packages=find_packages(),
packages=[package_name, package_name + '.filters'],

Choose a reason for hiding this comment

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

IIUC you can continue using find_packages() here (but I would test locally if rqt_reconfigure is happy with that).
The issue was that the package is nested in the src directory, in other places our folder structure is rqt_console/rqt_console/__init__.py instead of rqt_console/src/rqt_console/__init__.py.

In this case, with the src folder, you need the package_dir option below.
Sorry for the error in the previous review.

There's a package_dir option in setup.cfg if you prefer that one: https://setuptools.readthedocs.io/en/latest/userguide/package_discovery.html?highlight=find_packages#using-find-or-find-packages.

@ivanpauno
Copy link

LGTM with green CI that also tests rqt_reconfigure

@clalancette
Copy link

The change fixes the problem for me in local testing, so it looks OK to me.

However, I do have a question about this change and the change from #21. The branch that this is being merged into is dashing-devel, which is used for rqt_console on Dashing, Foxy, and Rolling. Is it intended that this fix (and the one from #21) will be put into all 3 distributions? If so, that is fine. If not, I think we'll need a new branch.

@clalancette
Copy link

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link

However, I do have a question about this change and the change from #21. The branch that this is being merged into is dashing-devel, which is used for rqt_console on Dashing, Foxy, and Rolling. Is it intended that this fix (and the one from #21) will be put into all 3 distributions? If so, that is fine. If not, I think we'll need a new branch.

Based on the discussion in the linked issue, targeting Dashing was considered ok: #20 (comment) (see the responses afterwards)

@clalancette
Copy link

Based on the discussion in the linked issue, targeting Dashing was considered ok: #20 (comment) (see the responses afterwards)

Ah, thanks for letting me know. In that case, with green CI, I'm going to go ahead and merge this. Thanks for the fix @ahcorde .

@clalancette clalancette merged commit f174bd9 into dashing-devel Jan 27, 2021
@clalancette clalancette deleted the ahcorde/fix/regression branch January 27, 2021 17:44
@tfoote tfoote mentioned this pull request Jul 14, 2022
arne48 pushed a commit that referenced this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants