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

Attempt to solve inconsistent results when using genmypy cmake hooks in catkin workspace #45

Merged

Conversation

mikaelarguedas
Copy link
Contributor

Hey there, thanks for the rospypi effort in general!

I attempted to use this in my ROS workspaces but had very inconsistent results.
I first thought that it was because the generation of the module files were dependant on the message/services pyi files being generated. While it helped a bit I still couldn't have reproducible builds.
Looking a little bit into it, it looks like the generation of the init.pyi files is dependant on already having a proper init.py file fully generated for the module. Is that a correct assumption ?

If so by using the changes in this PR I succeeded to have reproducible and more complete init.pyi files.
The PR basically introduced a dependency between genpy finishing generating message and module files for the genmypy file generation to kick in.

How I tested this:

  • Made a workspace with the common_msgs repo
  • 5 times:
    • Ran catkin_make_isolated
    • Looked at the number of lines of the generated pyi files

Using current master:

$ cat `find -name *.pyi` | wc -l
3747
$ cat `find -name *.pyi` | wc -l
3749
$ cat `find -name *.pyi` | wc -l
3762
$ cat `find -name *.pyi` | wc -l
3751
$ cat `find -name *.pyi` | wc -l
3736

Using this PR:

$ cat `find -name *.pyi` | wc -l
3765
$ cat `find -name *.pyi` | wc -l
3765
$ cat `find -name *.pyi` | wc -l
3765
$ cat `find -name *.pyi` | wc -l
3765
$ cat `find -name *.pyi` | wc -l
3765

Copy link
Member

@bonprosoft bonprosoft left a comment

Choose a reason for hiding this comment

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

I'm so sorry for my late response!!!

Thank you for your wonderful contribution, I'm really appreciated!
I confirmed that your patch works perfectly.
LGTM!

it looks like the generation of the init.pyi files is dependant on already having a proper init.py file fully generated for the module. Is that a correct assumption ?

I also looked into this problem and I am sure that your assumption is correct!

@bonprosoft
Copy link
Member

The CI error is totally unrelated to this PR. I will fix the issue, so please wait for a while 🙇‍♂️

@bonprosoft
Copy link
Member

I have fixed the issue at #46, so could you merge the latest master branch into your PR or rebase this branch from the master branch? Thank you!

…it file

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Contributor Author

I have fixed the issue at #46, so could you merge the latest master branch into your PR or rebase this branch from the master branch? Thank you!

Done 👍

@bonprosoft bonprosoft merged commit 72dff26 into rospypi:master Mar 16, 2023
@bonprosoft
Copy link
Member

Looks great! Thank you for your contribution!

@mikaelarguedas mikaelarguedas deleted the fix_custom_command_dependency branch March 16, 2023 06:59
@mikaelarguedas mikaelarguedas restored the fix_custom_command_dependency branch March 16, 2023 06:59
@mikaelarguedas
Copy link
Contributor Author

Thanks!

Do you think this can be released in noetic in the near future🙏 ?

@mikaelarguedas mikaelarguedas mentioned this pull request Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants