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 build with latest sdformat11 branch #607

Merged
merged 6 commits into from
Feb 3, 2021
Merged

Fix build with latest sdformat11 branch #607

merged 6 commits into from
Feb 3, 2021

Conversation

chapulina
Copy link
Contributor

The const-correctness introduced in gazebosim/sdformat#474 broke ign-gazebo. I originally thought this was a Windows-only issue, but it turns out it manifested first on Windows because CI builds SDFormat from source.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #607 (50c645c) into main (6166234) will decrease coverage by 0.03%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
- Coverage   77.38%   77.34%   -0.04%     
==========================================
  Files         213      213              
  Lines       11950    11967      +17     
==========================================
+ Hits         9247     9256       +9     
- Misses       2703     2711       +8     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
include/ignition/gazebo/World.hh 100.00% <ø> (ø)
include/ignition/gazebo/components/Factory.hh 96.05% <ø> (ø)
...e/ignition/gazebo/detail/EntityComponentManager.hh 95.00% <ø> (ø)
include/ignition/gazebo/gui/GuiRunner.hh 100.00% <ø> (ø)
include/ignition/gazebo/gui/GuiSystem.hh 0.00% <ø> (ø)
include/ignition/gazebo/gui/TmpIface.hh 0.00% <ø> (ø)
src/Model.cc 95.58% <ø> (ø)
src/Server.cc 82.80% <ø> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d50aceb...50c645c. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
auto light = root.Light();
light->SetName(desiredName);
entity = this->iface->creator->CreateEntities(light);
auto light = *root.Light();
Copy link

Choose a reason for hiding this comment

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

Hard to tell, does this cause a copy where there wasn't one before, or is root.Light() returning a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the idea is to make a copy, because Root::Light() returns a const pointer. I think this is only a problem on Windows. As far as I can tell, that has been returning a const pointer since it was implemented and didn't cause issues on Linux or macOS. That line was introduced in #572, before we enabled Windows CI.

@chapulina
Copy link
Contributor Author

I don't know why the Windows CI result is not coming back, but it built correctly. Test failures and warnings are expected.

Build Status

@adlarkin adlarkin self-requested a review February 3, 2021 20:32
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@chapulina chapulina merged commit a5d8762 into main Feb 3, 2021
@chapulina chapulina deleted the chapulina/5/win branch February 3, 2021 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants