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

pkg create --build-type ament_python should warn about hyphens in pkg-name #715

Open
krsche opened this issue May 21, 2022 · 7 comments · May be fixed by #717
Open

pkg create --build-type ament_python should warn about hyphens in pkg-name #715

krsche opened this issue May 21, 2022 · 7 comments · May be fixed by #717
Labels
help wanted Extra attention is needed

Comments

@krsche
Copy link

krsche commented May 21, 2022

Bug report

Required Info:

  • Operating System:
    • Ubuntu 22.04
  • Installation type:
    • binaries
  • Version or commit hash:
    • humble
  • DDS implementation:
    • /
  • Client library (if applicable):
    • ros2cli

Steps to reproduce issue

cd src
ros2 pkg create --build-type ament_python --license MIT --node-name hello-world  hello-world-python-1   # build breaks
ros2 pkg create --build-type ament_python --license MIT --node-name hello_world  hello-world-python-2   # build breaks
ros2 pkg create --build-type ament_python --license MIT --node-name hello-world  hello_world_python_3   # build breaks
ros2 pkg create --build-type ament_python --license MIT --node-name hello_world  hello_world_python_4   # build works
cd ..
colcon build --packages-select hello-world-python-1 # breaks
colcon build --packages-select hello-world-python-2 # breaks
colcon build --packages-select hello_world_python_3 # breaks
colcon build --packages-select hello_world_python_4 # works

Expected behavior

When running ros2 pkg create with --build-type ament_python warn about hyphens in the name / or abort pkg creation.

Actual behavior

No warning

Additional information

Not really a bug of ros2cli but also not really a feature request. I'd expect a warning, thats why I put it as a bug report.
I assume the check can easily be added here.

If there is agreement on that, I can open a PR with it. Please let me know :)

For reference: PEP-8: Package and Module Names

@aprotyas
Copy link
Member

I agree that a warning about the hyphen would be informative here!

@clalancette
Copy link
Contributor

For what it is worth, while using underscore is our best practice, I don't think hyphens should fail. I would prefer we figure out why hyphens are having problems.

@aprotyas
Copy link
Member

For what it is worth, while using underscore is our best practice, I don't think hyphens should fail. I would prefer we figure out why hyphens are having problems.

I think what's happening is that when you invoke ros2 pkg create --build-type ament_python --license MIT --node-name hello-world hello-world-python-1, a module named hello-world is created. I don't know where exactly it fails, but from what I understand modules with hyphens in the name can't be loaded/imported.

This thread touches on something tangentially related: https://stackoverflow.com/a/54599368

@clalancette
Copy link
Contributor

I think what's happening is that when you invoke ros2 pkg create --build-type ament_python --license MIT --node-name hello-world hello-world-python-1, a module named hello-world is created. I don't know where exactly it fails, but from what I understand modules with hyphens in the name can't be loaded/imported.

Oh, I see! Thanks for the explanation. OK, in that case I do agree that blocking them completely at ros2 pkg create time makes sense (for ament_python packages, at least).

(as a side note, I do believe that we can have hyphenated C++ packages, so for now I wouldn't change that semantic)

@krsche
Copy link
Author

krsche commented May 24, 2022

Yes agree, C++ works, but for python that's a language limitation afaik.
This is also mentioned in Pep-8.

I'll then happily open a PR for that :)

@krsche krsche linked a pull request May 25, 2022 that will close this issue
@clalancette clalancette added the help wanted Extra attention is needed label Jun 9, 2022
@808brick
Copy link

For what it's worth, it seems the issue you may be facing is not from the package name, but the setupcfg file that get's autogenerated.

It seems when the command is run, the setup.cfg file is created like so:

[develop]
script-dir=$base/lib/hello-world-python
[install]
install-scripts=$base/lib/hello-world-python

When it should be :

[develop]
script_dir=$base/lib/hello-world-python
[install]
install_scripts=$base/lib/hello-world-python

Where the difference is a underscore vs the hyphen. (Should be script_dir vs the autogenerated script-dir)

I hope this is helpful and related to resolving your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants