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.

Sign up for GitHub

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

Jump to bottom

Support empy3 and empy4 #821

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Support empy3 and empy4 #821

merged 7 commits into from
Aug 22, 2024
+69 −22

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 1, 2024

No description provided.

@ahcorde
Support empy3 and empy4
0accfa0 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Added feedback
6eed931 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
make linters happy
7951d47 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Aug 1, 2024
Open
@clalancette
Copy link
Contributor

I took this for a spin locally. I have good news and bad news.

The good news is that this works, and seems to keep compatibility with both Empy 3 and Empy 4.

The bad news is that with Empy 4, there is at least one bug. In particular, every time a rosidl_generator_py template file is expanded, it is printed to the console. This makes the output when building with it extremely noisy. I haven't dug into what is causing this, but I think we should find a way to fix/suppress this before we merge this in.

@clalancette
Remove some debugging code.
83cca77 
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 19, 2024

I took this for a spin locally. I have good news and bad news.

The good news is that this works, and seems to keep compatibility with both Empy 3 and Empy 4.

The bad news is that with Empy 4, there is at least one bug. In particular, every time a rosidl_generator_py template file is expanded, it is printed to the console. This makes the output when building with it extremely noisy. I haven't dug into what is causing this, but I think we should find a way to fix/suppress this before we merge this in.

I noticed this, That's why is still a draft

@ahcorde
Fixed stdout
c20cea3 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 20, 2024

@clalancette should be fixed now

@ahcorde ahcorde marked this pull request as ready for review August 20, 2024 10:44
@ahcorde
Removed trace
093f829 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
clalancette
clalancette reviewed Aug 21, 2024
View reviewed changes
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.

One more thought, then I think we are good to run CI on this.

rosidl_pycommon/rosidl_pycommon/__init__.py Outdated
if em_has_configuration:
interpreter.string(content, locals=kwargs)
else:
interpreter.string(content, str(template_path), kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I just noticed one more thing. This looks different from both line 243, and line 193 above. I'm wondering if we should just make this consistent with everything else:

Suggested change
interpreter.string(content, str(template_path), kwargs)
interpreter.string(content, template_path, locals=kwargs)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

761d831

@ahcorde
Include feedback
761d831 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

clalancette
clalancette approved these changes Aug 21, 2024
View reviewed changes
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 looks good to me, but I'm going to suggest we do a full CI run here (i.e. no --packages-above-and-dependencies). That's just because this is so deep in the rosidl pipeline, I want to make sure everything works.

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit e25750d into rolling Aug 22, 2024
4 checks passed
@ahcorde ahcorde deleted the ahcorde/rolling/empy4 branch August 22, 2024 08:29
@yashi
Copy link
Contributor

yashi commented Sep 14, 2024 •
edited
Loading

should we backport this to Jazzy, Iron, and Humble? Or they should use empy==3.3.4?

@fujitatomoya
Copy link

AFAIK, ubuntu nobel and jammy both use 3.3.4 so basically we do not use empy4. besides #821 (comment), we should wait until we make sure rolling works with em.Configuration without any problems. and then, we can consider backporting?

or do you have any specific requirement for this backporting?

@yashi
Copy link
Contributor

yashi commented Sep 16, 2024

No I don't. We can wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@cottsay cottsay Awaiting requested review from cottsay

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

@methylDragon methylDragon Awaiting requested review from methylDragon methylDragon is a code owner

@quarkytale quarkytale Awaiting requested review from quarkytale quarkytale is a code owner

@sloretz sloretz Awaiting requested review from sloretz sloretz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahcorde @clalancette @yashi @fujitatomoya