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

Compilation fixes for Windows - codecheck patches (part 1) #501

Merged
merged 30 commits into from
Jan 25, 2021
Merged

Compilation fixes for Windows - codecheck patches (part 1) #501

merged 30 commits into from
Jan 25, 2021

Conversation

j-rivero
Copy link
Contributor

This is the first part of Windows fixes to make ign-gazebo compile, commits are self-explanatory but in a summary:

  • Missing headers, including ones reported by codecheck
  • Portable setenv from ignition::common
  • Visibility keywords
  • Portability fixes for cmake

Compilation is not yet there, this is the first part of patches. Let's see how it goes for UNIX ci.

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Dec 18, 2020
@j-rivero
Fix for Protobuf compiler
4719b87 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
No symlink on Windows
b860682 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Add missing <optional> headers
25150a4 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Missing header for accumulate
d8f5da7 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fix error with experimental on MSVC
3555c8f 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Add missing visible macros
a77513c 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fixes for Windows in CMake
caaa192 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Portable setenv/getenv
a66a2d0 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fix API for setenv
c0a291b 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Protobuf requires no visibility keywords
9a2b071 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fix use of filesystem on Windows
3a48576 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Workaround for getMessage Windows call
d6e88fa 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Include codecheck fixes
591980e 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fix style for comment
1562680 
Signed-off-by: Jose Luis Rivero <[email protected]>
@chapulina
Copy link
Contributor

Nice! This one too, do you think it would be possible to target ign-gazebo3?

@chapulina chapulina added the Windows Windows support label Dec 18, 2020
@nkoenig nkoenig changed the base branch from main to ign-gazebo5 December 23, 2020 23:33
@j-rivero
Copy link
Contributor Author

j-rivero commented Jan 4, 2021

Nice! This one too, do you think it would be possible to target ign-gazebo3?

I need to check if visibility is affected somehow with these changes on top of ign-gazebo3

@chapulina chapulina changed the base branch from ign-gazebo5 to main January 5, 2021 00:09
chapulina
chapulina reviewed Jan 12, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The bump to ign-msgs7 and ign-transport10 is being done in #546

src/systems/CMakeLists.txt Outdated Show resolved Hide resolved
src/systems/multicopter_control/LeeVelocityController.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/EventManager.hh Show resolved Hide resolved
@j-rivero
Copy link
Contributor Author

j-rivero commented Jan 14, 2021 •
edited
Loading

The bump to ign-msgs7 and ign-transport10 is being done in #546

Thanks, I've merged main into this PR.

@j-rivero
Copy link
Contributor Author

Nice! This one too, do you think it would be possible to target ign-gazebo3?

I ported the changes to ign-gazebo3 branch and If I did not fail during the process there are problems with ABI stability. I'm afraid that changes are too intrusive to be backported.

@chapulina
Copy link
Contributor

I'm afraid that changes are too intrusive to be backported.

Ah ok, thanks for trying. Let's keep targeting Edifice then.

@j-rivero j-rivero mentioned this pull request Jan 15, 2021
Merged
@codecov
Copy link

codecov bot commented Jan 19, 2021 •
edited
Loading

Codecov Report

Merging #501 (5f23c9c) into main (6166234) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #501   +/-   ##
=======================================
  Coverage   77.38%   77.38%           
=======================================
  Files         213      213           
  Lines       11950    11951    +1     
=======================================
+ Hits         9247     9248    +1     
  Misses       2703     2703
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
include/ignition/gazebo/World.hh 100.00% <ø> (ø)
...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/Server.cc 82.80% <ø> (ø)
src/SimulationRunner.hh 100.00% <ø> (ø)
include/ignition/gazebo/EventManager.hh 79.16% <100.00%> (+0.90%) ⬆️

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 05b0126...5f23c9c. Read the comment docs.

@j-rivero j-rivero requested a review from chapulina January 19, 2021 23:43
@j-rivero j-rivero mentioned this pull request Jan 20, 2021
Open
18 tasks
@chapulina chapulina merged commit 63a88dc into main Jan 25, 2021
@chapulina chapulina deleted the win_pr1 branch January 25, 2021 16:19
This was referenced Mar 1, 2021
Merged
Merged
@Tobias-Fischer Tobias-Fischer mentioned this pull request Jun 13, 2021
Merged
5 tasks
@chapulina chapulina mentioned this pull request Aug 5, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@iche033 iche033 Awaiting requested review from iche033

Assignees
No one assigned
Labels
🏢 edifice Ignition Edifice Windows Windows support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@j-rivero @chapulina