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

Change installation for message generation #3

Merged
merged 5 commits into from
Jul 11, 2023
Merged

Change installation for message generation #3

merged 5 commits into from
Jul 11, 2023

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Jun 26, 2023 •
edited
Loading

Install the generation scripts and compiler in the dev package.

Install the protos in the main package.

No longer installing ruby.

Should fix debbuilder failures such as https://build.osrfoundation.org/view/ign-harmonic/job/gz-msgs10-debbuilder/298/console

@mjcarroll
Change installation for message generation
73e93f0 
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@azeey
Copy link
Contributor

azeey commented Jun 26, 2023

@caguero any reason the default branch is not main?

@j-rivero j-rivero self-assigned this Jun 26, 2023
@j-rivero
Copy link
Contributor

The file installed by the job is there:
/home/jenkins/workspace/gz-msgs10-debbuilder/build/gz-msgs/debian/tmp/usr/lib/ruby/gz/cmdmsgs10.rb

In the release info there are a couple of packages hosting ruby files:

gz-msgs10-release/jammy/debian on  main ❯ grep -R ruby *
libgz-msgs10-dev.install:usr/lib/*/ruby/*/msgs[0-99]*/*
libgz-msgs10.install:usr/lib/ruby/gz/*.rb

The first one in -dev is wrong, so removing it in this PR make it work. In msgs9 I see many files *_pb.rb :

-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/raysensor_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/request_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/response_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/rest_login_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/rest_logout_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/rest_post_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/rest_response_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/road_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/scene_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/sdf_generator_config_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/selection_pb.rb
-- Installing: /home/jenkins/workspace/gz-msgs9-debbuilder/build/gz-msgs-9.4.0/debian/tmp/usr/lib/aarch64-linux-gnu/ruby/gz/msgs9/sensor_pb.rb

Should be generating them or something has change that justify the lack of them?

@Crola1702 Crola1702 mentioned this pull request Jun 28, 2023
Closed
@mjcarroll
Copy link
Contributor Author

Ah, we still need to install the cmdmsgs.rb script, I will adjust for that. The remainder are the generated code for the ruby messages, which is not happening in msgs10 anymore. Rather, we are relying on downstream packages to generate their own code from the proto definitions.

scpeters
scpeters reviewed Jun 29, 2023
View reviewed changes
ubuntu/debian/libgz-msgs.install
@@ -1,4 +1,4 @@
usr/lib/*/*.so.*
usr/lib/ruby/gz/*.rb
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just revert this line and it should work

Copy link
Contributor

Choose a reason for hiding this comment

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

ab1399f

@scpeters
libgz-msgs.install: install cmd*.rb file
ab1399f 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Contributor

@caguero any reason the default branch is not main?

this is important because the nightly debs are currently building from main:

@caguero
Copy link
Collaborator

caguero commented Jun 29, 2023

I just updated the default branch to main. I think you could retarget this PR now.

@scpeters scpeters changed the base branch from caguero/msgs10 to main June 29, 2023 17:35
@scpeters
Copy link
Contributor

I just updated the default branch to main. I think you could retarget this PR now.

thanks, I'll resolve the conflicts

@scpeters
Copy link
Contributor

resolved conflicts, starting a test build:

@scpeters
Copy link
Contributor

the test build was unstable; we need an extra line to handle installing the cmake extras

@scpeters
libgz-msgs-dev.install: install cmake extras
db5dd0b 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Contributor

after db5dd0b, the new test build is fixed:

@scpeters scpeters mentioned this pull request Jun 30, 2023
Merged
8 tasks
@scpeters
Copy link
Contributor

I think all the cmake files should be installed to the same folder: gazebosim/gz-cmake#360

@scpeters scpeters mentioned this pull request Jun 30, 2023
Merged
5 tasks
@scpeters
Copy link
Contributor

I think all the cmake files should be installed to the same folder: gazebosim/gz-cmake#360

also, there may be an issue with the relative-path logic in https://github.com/gazebosim/gz-msgs/pull/339/files#r1247319505

@scpeters
Revert line in libgz-msgs-dev.install
2d84b0a 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Contributor

scpeters commented Jul 7, 2023

I have a fix for the relative path logic in gazebosim/gz-msgs#359 and have started a nightly debbuild from that branch

@scpeters scpeters mentioned this pull request Jul 7, 2023
Closed
@j-rivero j-rivero mentioned this pull request Jul 7, 2023
Closed
@scpeters
Copy link
Contributor

I think this is ready to merge. I approved it but also committed to it so I'll leave it to someone else to merge

@j-rivero
Copy link
Contributor

I think this is ready to merge. I approved it but also committed to it so I'll leave it to someone else to merge

Thanks Steve, merging!

@j-rivero j-rivero merged commit 1d039fe into main Jul 11, 2023
@j-rivero j-rivero deleted the mjcarroll/message_generation_updates branch July 11, 2023 10:40
@mjcarroll mjcarroll mentioned this pull request Jul 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

Assignees

@j-rivero j-rivero

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mjcarroll @azeey @j-rivero @scpeters @caguero