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

CMake improvements for Windows build #3611

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

claudiofantacci
Copy link
Contributor

This PR is a followup of issue #3597 that tries to address the problem of #3483 and to add CMAKE_DEBUG_POSTFIX when compiling with MS Visual Studio and emulators (those that are recognized by CMake as MSVC generators).

The main difference lies in third-party/libtm/resources/CMakeLists.txt where multiple file(APPEND ...) commands are replaced with a single file(WRITE ...) command. This is to resolve a problem where sometimes the files where malformed.
⚠️ Note that strange indentation of the file(WRITE ...) commands is necessary to have a proper indentation in the generated files. If you don't like it (I personally don't 😄) I reformat the string into a single line by adding \ns, but I think the result is even uglier than the former and much less readable 🙃.

I also added two new entries in the .gitignore file:

  1. exclude all folders that starts with build, for example I may have a folder build for Release and a buildd folder for Debug.
  2. exclude the doxygen-generated documentation files.

Note that in order to close #3597 we also need #3507 to be merged.

Fixes #3483.

@dorodnic
Copy link
Contributor

dorodnic commented Apr 2, 2019

Thanks @claudiofantacci
I tested this PR merged together with #3647 and everything seem to work.
Really like the d suffix, as it will save a rebuild when switching between Debug and Release modes.
We might have to push it to the next release, since the suffix will require changes to Windows SDK Installer on our part, but otherwise looks good to me 👍

@claudiofantacci
Copy link
Contributor Author

Thanks for your feedback @dorodnic, really appreciated 🎉

Really like the d suffix, as it will save a rebuild when switching between Debug and Release modes.
We might have to push it to the next release since the suffix will require changes to Windows SDK Installer on our part, but otherwise looks good to me 👍

This. Is. Awesome!
Thanks again and looking forward to see this PR merged 👍
Let me know if you need any changes!

@ev-mp
Copy link
Collaborator

ev-mp commented Apr 18, 2019

@claudiofantacci , the mentioned merge resulted in a merge conflicts with this PR.
Can you update/rebase it to accommodate for the changes ? Thanks

@claudiofantacci
Copy link
Contributor Author

@ev-mp sure I will! I’m out of office now, I’ll do everything tomorrow 🙂

@radfordi
Copy link
Contributor

This needs rebasing, @claudiofantacci, now that #3647 has been merged.

@claudiofantacci
Copy link
Contributor Author

I just rebased on top of development branch.
By my understanding, the write(FILE) CMake command of the libtm CMake code has been deleted. As a consequence, I removed the commit that was modifying that part of the code.
If you want, I will edit the first post to adjust the content to the two remaining commits.

@radfordi
Copy link
Contributor

Thanks @claudiofantacci. Yes, the write(FILE) is gone now. 7a63691 looks like it's a fix to only use MSVC options when actually compiling with MSVC (and not say with clang) combined with the addition of the d. Is that correct?

@claudiofantacci
Copy link
Contributor Author

@radfordi exactly! Only those two points 🙂

@dorodnic
Copy link
Contributor

@claudiofantacci thank you for contributing!
Your feedback, help with review, and keeping patches up to date are all very much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants