Skip to content
This repository was archived by the owner on Feb 3, 2025. It is now read-only.

Add support for compiling on windows x64 and x86 with vcpkg-provided dependencies#3320

Merged
traversaro merged 9 commits into
gazebosim:gazebo11from
talregev:TalR/x86
Sep 15, 2023
Merged

Add support for compiling on windows x64 and x86 with vcpkg-provided dependencies#3320
traversaro merged 9 commits into
gazebosim:gazebo11from
talregev:TalR/x86

Conversation

@talregev
Copy link
Copy Markdown
Contributor

@talregev talregev commented Apr 29, 2023 •
edited
Loading

Compile for windows x64 and x86.
Upload the artifacts. You can download and test it yourself.
x64 not running.
x86 only splash appear:
x86 full running!
image

@traversaro
I know I get full ignore because I open an issue telling that here we get a low responds from maintainers.
Even with your ignore, I am still right, and I am still keep develop for gazebo classic.
This things that I bring here, can done also for the new gazebo with the help of the community.

@traversaro traversaro mentioned this pull request May 13, 2023
Closed
@traversaro
Copy link
Copy Markdown
Collaborator

traversaro commented May 13, 2023

I know I get full ignore because I open an issue telling that here we get a low responds from maintainers.

Actually I had just lost this notification.

@talregev talregev force-pushed the TalR/x86 branch 2 times, most recently from d72bbf8 to 2396e61 Compare June 17, 2023 09:17
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 17, 2023

@traversaro
Can I do anything to help getting this PR?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 18, 2023

the gzserver.exe seems to work. When I open it separately it run without crash, then:
gz topic -l
image

So it compile correctly.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 18, 2023 •
edited
Loading

I was able to run gzclient in x86!
I did gzclient.exe --verbose to show what it missing.
It was missing this folder: C:\Users\<my user name>\.gazebo\
And missing HOME variable
and I create HOME variable to:
set HOME=C:\Users\<my user name>

EDIT: The same way I was able to run gazebo.exe

Then it show up!
image

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 18, 2023 •
edited
Loading

@traversaro
For windows it shouldn't search for HOME variable. it should search for USERPROFILE.
Also it should create automatic this folder:
C:\Users\<my user name>\.gazebo\

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 18, 2023

image

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jun 28, 2023

@traversaro How I can help to move this PR forward? It already running.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jul 19, 2023

@scpeters Can you take a look?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jul 22, 2023

@scpeters Can you test locally that I didn't break the regular gazebo classic build?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Jul 28, 2023

@traversaro @scpeters
How I can help to move this PR forward?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 4, 2023

@traversaro Thank you for the merge.
Can you take a look on this PR also?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 11, 2023

@traversaro Any reason to block this PR?
We can discuss and change stuff to move this forward.
Is it something you want that you can compile gazebo without conda?

@traversaro
Copy link
Copy Markdown
Collaborator

traversaro commented Aug 14, 2023

@traversaro Any reason to block this PR? We can discuss and change stuff to move this forward. Is it something you want that you can compile gazebo without conda?

Sorry for the late reply, I hve some doubts about the mantainability of pinning ogre, qwt and graphviz without pinning the other dependencies. Anyhow, we can discuss about this with the other mantainers. In the meanwhile I can review the rest of the PR, that probably can also merged without the vcpkg.json and related CI.

@traversaro
Copy link
Copy Markdown
Collaborator

traversaro commented Aug 14, 2023

Just to understand, x64 is still not running, right?

traversaro
traversaro reviewed Aug 14, 2023
View reviewed changes
Comment thread .github/workflows/windows.yml Outdated
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 14, 2023 •
edited
Loading

The x64 is still not working.
I can remove it from running on the ci if you wish.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 14, 2023

@traversaro Any reason to block this PR? We can discuss and change stuff to move this forward. Is it something you want that you can compile gazebo without conda?

Sorry for the late reply, I hve some doubts about the mantainability of pinning ogre, qwt and graphviz without pinning the other dependencies. Anyhow, we can discuss about this with the other mantainers. In the meanwhile I can review the rest of the PR, that probably can also merged without the vcpkg.json and related CI.

All the dependencies are already pinned.
They pinned to a specific commit of vcpkg.
It means they do not get the latest update and stay the same always.

traversaro
traversaro reviewed Aug 14, 2023
View reviewed changes
Comment thread cmake/FindFreeimage.cmake Outdated
traversaro
traversaro reviewed Aug 14, 2023
View reviewed changes
Comment thread cmake/SearchForStuff.cmake
traversaro
traversaro reviewed Aug 14, 2023
View reviewed changes
Comment thread cmake/SearchForStuff.cmake Outdated
traversaro
traversaro reviewed Aug 14, 2023
View reviewed changes
Comment thread cmake/SearchForStuff.cmake Outdated
traversaro
traversaro reviewed Aug 16, 2023
View reviewed changes
Comment thread cmake/SearchForStuff.cmake Outdated
@talregev
bug fix:
9d7660f 
- revert tbb changes
- unset(freeimage_FOUND CACHE)
- unset(tinyxml_FOUND CACHE)
- set(Simbody_LIBRARIES ${Simbody_STATIC_LIBRARIES}) when ${Simbody_LIBRARIES} not found. this is static build case
@talregev
Review comments:
72ae3b7 
- revert changes of freeimages
- revert changes of tinyxml
@talregev talregev requested a review from traversaro August 16, 2023 19:08
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 16, 2023

@traversaro
Ready for review.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 22, 2023

@traversaro
Gentle remind you about this PR.
Also if you can update status or what you do next, it will be nice.
Thank you for your time.

traversaro
traversaro reviewed Aug 22, 2023
View reviewed changes
Comment thread cmake/SearchForStuff.cmake Outdated
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 25, 2023

@traversaro Thank you for your comments. I am also agree to minimal and small PRs.
This is Gentle remind you about this PR.
Anything I can do to move it forward. I think it the minimal changes for this PR.
I will also happy to know the current status of this PR and if it need to have more approvals.

Thank you for your time and effort. We both will make gazebo better.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 4, 2023

@traversaro Any update?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 8, 2023

@traversaro gentle remind you about this PR.
btw, the problem of windows conda fix itself or this PR fix it by accident?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 14, 2023

@traversaro Can you take a look?

scpeters
scpeters reviewed Sep 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

thank you for your patience with this pull request process. This project is in maintenance mode, so it is not a high priority for us, but I appreciate the work that you have done to demonstrate how to use gazebo-classic on windows with vcpkg.

I think the remaining changes to CMakeLists.txt and cmake/SearchForStuff.cmake don't look harmful, and I don't mind adding the vcpkg metadata if it supports some users.

I looked at the artifact size, and there are 2 uploads of about 500 MB each; I don't think we should upload artifacts of this size

Otherwise, I think we're almost there, pending final thoughts from @traversaro

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 14, 2023

thank you for your patience with this pull request process. This project is in maintenance mode, so it is not a high priority for us, but I appreciate the work that you have done to demonstrate how to use gazebo-classic on windows with vcpkg.

I think the remaining changes to CMakeLists.txt and cmake/SearchForStuff.cmake don't look harmful, and I don't mind adding the vcpkg metadata if it supports some users.

I looked at the artifact size, and there are 2 uploads of about 500 MB each; I don't think we should upload artifacts of this size

Otherwise, I think we're almost there, pending final thoughts from @traversaro

Thank you for your replay. I remove the upload. Please review again.

traversaro
traversaro approved these changes Sep 15, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Ok for me, thanks for the PR!

@traversaro traversaro changed the title Compile for windows x64 and x86 Add support for comping on windows x64 and x86 with vcpkg-provided dependencies Sep 15, 2023
@traversaro
Copy link
Copy Markdown
Collaborator

traversaro commented Sep 15, 2023

@talregev I proposed a more descriptive name for the PR, if you want to change it further feel free to do it!

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 15, 2023

@talregev I proposed a more descriptive name for the PR, if you want to change it further feel free to do it!

That great from my side!
Any thing I can do to move this PR further?

@traversaro traversaro merged commit f398583 into gazebosim:gazebo11 Sep 15, 2023
@traversaro
Copy link
Copy Markdown
Collaborator

traversaro commented Sep 15, 2023

@talregev I proposed a more descriptive name for the PR, if you want to change it further feel free to do it!

That great from my side! Any thing I can do to move this PR further?

No, merged!

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 15, 2023

@talregev I proposed a more descriptive name for the PR, if you want to change it further feel free to do it!

That great from my side! Any thing I can do to move this PR further?

No, merged!

Thank you!

@talregev talregev deleted the TalR/x86 branch September 15, 2023 12:00
@traversaro traversaro changed the title Add support for comping on windows x64 and x86 with vcpkg-provided dependencies Add support for compiling on windows x64 and x86 with vcpkg-provided dependencies Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

1 more reviewer

@traversaro traversaro traversaro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@talregev @traversaro @scpeters