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

Correcting Cmake files for Windows and Visual Studio, and adding d suffix for the generated library in debug mode #1395

Merged
merged 6 commits into from
Feb 28, 2019

Conversation

raphaellenain
Copy link
Contributor

@raphaellenain raphaellenain commented Feb 19, 2019

PR Description

Adding a debug version of Library (DGtald.lib)
Fixing CMake files for Visual Studio and Windows paths (adding quotes for paths with spaces like "C:/Program Files/DGtal")

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@dcoeurjo dcoeurjo self-requested a review February 19, 2019 10:47
@dcoeurjo dcoeurjo self-assigned this Feb 19, 2019
@dcoeurjo dcoeurjo added the Build label Feb 19, 2019
@dcoeurjo dcoeurjo added this to Inbox in Issue triage via automation Feb 19, 2019
@dcoeurjo dcoeurjo added this to the 1.0 milestone Feb 19, 2019
@dcoeurjo
Copy link
Member

Thanks @raphaellenain for the PR.

Just a quick clarification: doc and dox are two specific targets in the cmake.
The first one builds the entire doc while the second one only builds the module/package doc pages. Both are thus valid.

(Not sure btw if the dox target is oftenly used...)

Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, I have couple of pending questions concerning your edits. Let’s discsuss about them here ;)

cmake/Common.cmake Outdated Show resolved Hide resolved
cmake/FindFFTW3.cmake Show resolved Hide resolved
cmake/TargetDoxygenDox.cmake Outdated Show resolved Hide resolved
examples/tutorial-examples/CMakeLists.txt Show resolved Hide resolved
@raphaellenain
Copy link
Contributor Author

I don't understand the error on Travis ... it's written "canceled", I don't know why ... Any idea ?

@dcoeurjo
Copy link
Member

Some admin stuff on Travis side. If you log in on Travis, you can restart the job

Issue triage automation moved this from Inbox to Done Feb 20, 2019
@raphaellenain raphaellenain reopened this Feb 20, 2019
Issue triage automation moved this from Done to In progress Feb 20, 2019
@dcoeurjo dcoeurjo self-requested a review February 27, 2019 12:25
# -----------------------------------------------------------------------------
# To distinguish between debug and release lib
# -----------------------------------------------------------------------------
set(CMAKE_DEBUG_POSTFIX "d")
Copy link
Member

Choose a reason for hiding this comment

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

Would this also apply on linux/mac builds ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Linux, if we want the indicate the debug or release configuration, the "-DCMAKE_BUILD_TYPE" option has to be added :

  • for Debug : "cmake -DCMAKE_BUILD_TYPE=Debug"
  • for Release : "cmake -DCMAKE_BUILD_TYPE=Debug" (or "cmake", Release is the default configuration)
    With this new line in Common.cmake(set(CMAKE_DEBUG_POSTFIX "d")) :
  • for Debug configuration, the generated file is libdgtald.so (on Windows : DGtald.lib)
  • for Release configuration, the generated file is libdgtal.so (on Windows : DGtal.lib)

For Mac I cannot perform any test, but I imagine it's the same thing than on Linux.

Copy link
Member

Choose a reason for hiding this comment

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

I know that but was just wondering if the "d" suffix would also be used by cmake on linux for the dynamic lib construction.

Copy link
Member

Choose a reason for hiding this comment

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

which is not standard for me

Copy link
Member

Choose a reason for hiding this comment

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

(and would imply a tweak of the FindDGtal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing some tests for an alternative solution (without changing DGtal Cmake file :-) ). But the 2 errors on Visual studio are "_ITERATOR_DEBUG_LEVEL" and "RuntimeLibrary" : "https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2038?view=vs-2017"

Copy link
Member

Choose a reason for hiding this comment

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

You can edit the cmake if you want (only for MSVC). I don't undrestand why VC complains but not a big deal if that fixes the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I succeeded compiling my project in Debug mode, referencing the Release version of DGtal.
When creating a new project on Visual Studio, the IDE automatically :

  • for the Debug configuration : adds a preprocessor macro "_DEBUG" (--> _ITERATOR_DEBUG_LEVEL = 2), and sets the property "UseDebugLibraries" to true (--> RuntimeLibrary : Debug)
  • for the Release configuration : adds a preprocessor macro "NDEBUG" (--> _ITERATOR_DEBUG_LEVEL = 0), and sets the property "UseDebugLibraries" to false (--> RuntimeLibrary : Release)

So compiling in Debug mode, linking to the Release version of DGtal, is possible, changing these configurations, but it's not the default configuration for Visual Studio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I add the "MSVC" test

Copy link
Member

Choose a reason for hiding this comment

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

👌

@dcoeurjo
Copy link
Member

Thanks for the edits. Could you please merge the master to this branch and fix the conflict in Changelog.md?

@dcoeurjo
Copy link
Member

could you please update the AUTHORS file as well ;) ?

@dcoeurjo
Copy link
Member

👌

@raphaellenain
Copy link
Contributor Author

Thank you for the code review

@dcoeurjo
Copy link
Member

thanks to you for the PR.
Merging

@dcoeurjo dcoeurjo merged commit 877ef62 into DGtal-team:master Feb 28, 2019
Issue triage automation moved this from In progress to Done Feb 28, 2019
@raphaellenain raphaellenain deleted the RLEVisual2017Adaptation branch April 1, 2019 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Issue triage
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants