github-starred/libxlsxwriter
2
0
Fork 0
mirror of https://github.com/jmcnamara/libxlsxwriter.git synced 2026-05-15 14:15:54 -06:00
Code Issues 16 Projects Packages Wiki Activity Actions

[GH-ISSUE #177] Don't hardcode msvc runtime library in cmake file #143

New issue
Closed
opened 2026-05-05 11:46:31 -06:00 by gitea-mirror · 10 comments
gitea-mirror commented 2026-05-05 11:46:31 -06:00
Owner
Copy link

Originally created by @dirkvdb on GitHub (May 15, 2018).
Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/177

Originally assigned to: @jmcnamara on GitHub.

When compiling with MSVC the runtime settings are hard coded in the CMakeLists.txt

set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} /MTd /O0 /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb")
set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /MT /Ox /Zi /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb")
set(CMAKE_C_FLAGS_MINSIZEREL "${CMAKE_C_FLAGS_MINSIZEREL} /MT /Zi /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb")
set(CMAKE_C_FLAGS_RELWITHDEBINFO "${CMAKE_C_FLAGS_RELWITHDEBINFO} /MT /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb")

This is bad practice, these settings should be defined in a toolchain file, never hardcoded in the cmake file.

When i want to compile the library using a different runtime for inclusion in my project the only option I have is patching the CMakeLists.txt

Originally created by @dirkvdb on GitHub (May 15, 2018). Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/177 Originally assigned to: @jmcnamara on GitHub. When compiling with MSVC the runtime settings are hard coded in the CMakeLists.txt ``` set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} /MTd /O0 /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb") set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /MT /Ox /Zi /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb") set(CMAKE_C_FLAGS_MINSIZEREL "${CMAKE_C_FLAGS_MINSIZEREL} /MT /Zi /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb") set(CMAKE_C_FLAGS_RELWITHDEBINFO "${CMAKE_C_FLAGS_RELWITHDEBINFO} /MT /Fd${CMAKE_BINARY_DIR}/${PROJECT_NAME}.pdb") ``` This is bad practice, these settings should be defined in a toolchain file, never hardcoded in the cmake file. When i want to compile the library using a different runtime for inclusion in my project the only option I have is patching the CMakeLists.txt
gitea-mirror 2026-05-05 11:46:31 -06:00
gitea-mirror commented 2026-05-05 11:46:38 -06:00
Author
Owner
Copy link

@jmcnamara commented on GitHub (May 15, 2018):

@Alexhuszagh can you have a look at this as well.

<!-- gh-comment-id:389241846 --> @jmcnamara commented on GitHub (May 15, 2018): @Alexhuszagh can you have a look at this as well.
gitea-mirror commented 2026-05-05 11:46:41 -06:00
Author
Owner
Copy link

@jmcnamara commented on GitHub (May 16, 2018):

@dirkvdb If you know how to fix this and can submit a PR then that would be good.

<!-- gh-comment-id:389433974 --> @jmcnamara commented on GitHub (May 16, 2018): @dirkvdb If you know how to fix this and can submit a PR then that would be good.
gitea-mirror commented 2026-05-05 11:46:43 -06:00
Author
Owner
Copy link

@dirkvdb commented on GitHub (May 16, 2018):

I created a pull request, all it does is remove the hardcoded runtime flags.

<!-- gh-comment-id:389440748 --> @dirkvdb commented on GitHub (May 16, 2018): I created a pull request, all it does is remove the hardcoded runtime flags.
gitea-mirror commented 2026-05-05 11:46:45 -06:00
Author
Owner
Copy link

@jmcnamara commented on GitHub (May 16, 2018):

For reference: PR #178

<!-- gh-comment-id:389530578 --> @jmcnamara commented on GitHub (May 16, 2018): For reference: PR #178
gitea-mirror commented 2026-05-05 11:46:49 -06:00
Author
Owner
Copy link

@jmcnamara commented on GitHub (May 16, 2018):

@RalfKubis Any thoughts on this?

<!-- gh-comment-id:389531858 --> @jmcnamara commented on GitHub (May 16, 2018): @RalfKubis Any thoughts on this?
gitea-mirror commented 2026-05-05 11:46:53 -06:00
Author
Owner
Copy link

@Alexhuszagh commented on GitHub (May 17, 2018):

@dirkvdb It is bad practice, but greatly simplifies life for simple cases.
@jmcnamara They're right, and I would likely accept the PR.

I can document toolchain configurations (although manually providing them is likely beyond the scope of this project, although I would gladly do so) inside the CMakeLists as a follow-up PR.

<!-- gh-comment-id:389913004 --> @Alexhuszagh commented on GitHub (May 17, 2018): @dirkvdb It is bad practice, but greatly simplifies life for simple cases. @jmcnamara They're right, and I would likely accept the PR. I can document toolchain configurations (although manually providing them is likely beyond the scope of this project, although I would gladly do so) inside the CMakeLists as a follow-up PR.
gitea-mirror commented 2026-05-05 11:46:57 -06:00
Author
Owner
Copy link

@dirkvdb commented on GitHub (May 17, 2018):

@Alexhuszagh

  • I find it strange that you apply the runtime setting only for static library builds. Building a static library does not imply that you use the static runtime. It is perfectly feasible to build static libraries against the dynamic runtime.
  • The way the setting is now applied is not really clean, you add the runtime compiler option to the already present compiler options, resulting in both the dynamic and static runtime flag to be present in the compiler flags, you are lucky msvc takes the latter one
    It would be better to do:
    string(REPLACE "/MD" "/MT" ${CMAKE_C_FLAGS} "${${CMAKE_C_FLAGS}}")
  • If you want I can change the pull request to add an option like FORCE_STATIC_MSVC_RUNTIME, that way you can get the current behaviour without messing with toolchains.
<!-- gh-comment-id:389953274 --> @dirkvdb commented on GitHub (May 17, 2018): @Alexhuszagh - I find it strange that you apply the runtime setting only for static library builds. Building a static library does not imply that you use the static runtime. It is perfectly feasible to build static libraries against the dynamic runtime. - The way the setting is now applied is not really clean, you add the runtime compiler option to the already present compiler options, resulting in both the dynamic and static runtime flag to be present in the compiler flags, you are lucky msvc takes the latter one It would be better to do: `string(REPLACE "/MD" "/MT" ${CMAKE_C_FLAGS} "${${CMAKE_C_FLAGS}}")` - If you want I can change the pull request to add an option like `FORCE_STATIC_MSVC_RUNTIME`, that way you can get the current behaviour without messing with toolchains.
gitea-mirror commented 2026-05-05 11:47:02 -06:00
Author
Owner
Copy link

@Alexhuszagh commented on GitHub (May 17, 2018):

@dirkvdb I entirely agree, also, this is not my code in question. We may want to ask @jimzshi what he thinks, since he authored the PR.

I think we should wait for feedback from him, but I agree this is peculiar, and also not best practice. Options sound like a great way, but this does honestly sound like something for a toolchain.

For reference:
https://github.com/jmcnamara/libxlsxwriter/pull/103

9a78c323a9 (diff-af3b638bc2a3e6c650974192a53c7291)

<!-- gh-comment-id:389987443 --> @Alexhuszagh commented on GitHub (May 17, 2018): @dirkvdb I entirely agree, also, this is not my code in question. We may want to ask @jimzshi what he thinks, since he authored the PR. I think we should wait for feedback from him, but I agree this is peculiar, and also not best practice. Options sound like a great way, but this does honestly sound like something for a toolchain. For reference: https://github.com/jmcnamara/libxlsxwriter/pull/103 https://github.com/jmcnamara/libxlsxwriter/commit/9a78c323a925c75d8feffb7c3f6a7773e79d2628#diff-af3b638bc2a3e6c650974192a53c7291
gitea-mirror commented 2026-05-05 11:47:04 -06:00
Author
Owner
Copy link

@jimzshi commented on GitHub (May 18, 2018):

@Alexhuszagh @dirkvdb

i have no objections removing runtime lib from cmakefile.

imho both approach has its up/down sides in different scenarios. at this stage, i reckon @dirkvdb proposal is better since it provides more flexibility to the lib compilation.

i also think it would be even better if @dirkvdb can at meanwhile provide cmake options that lib users can choose runtime lib as they wish. many thanks in advance.

cheers.

<!-- gh-comment-id:390144580 --> @jimzshi commented on GitHub (May 18, 2018): @Alexhuszagh @dirkvdb i have no objections removing runtime lib from cmakefile. imho both approach has its up/down sides in different scenarios. at this stage, i reckon @dirkvdb proposal is better since it provides more flexibility to the lib compilation. i also think it would be even better if @dirkvdb can at meanwhile provide cmake options that lib users can choose runtime lib as they wish. many thanks in advance. cheers.
gitea-mirror commented 2026-05-05 11:47:06 -06:00
Author
Owner
Copy link

@jmcnamara commented on GitHub (May 21, 2018):

Merged. Thanks.

<!-- gh-comment-id:390638337 --> @jmcnamara commented on GitHub (May 21, 2018): Merged. Thanks.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
coverity
cmake-improvements
cmake_pkgconfig_fix
ctest
button
tables
autofilter
dtoa
iwyu
openssl
gentoo
gh310
condition_format
rich_string_html
header_footer_images
custom_data_labels
object_position
gh265
constant_mode_comments2
constant_mode_comments
vml
md5
vcpkg
hyperlinks
api_refactor
win_fopen
gh203
rich_strings
chartsheet
in_mem_image
gh192
revert-162-master
data_validation
meson
charts
scan_build
utf8_strlen
tmpfileplus
custom_properties
insert_image
default_row
control_chars
worksheet_protect
dev
red_black
panes
mkstemp
defined_names
optimise
wip
v1.2.4
v1.2.3
v1.2.2
v1.2.1
v1.2.0
v1.1.9
v1.1.8
RELEASE_1.1.7
RELEASE_1.1.6
RELEASE_1.1.5
RELEASE_1.1.4
RELEASE_1.1.3
RELEASE_1.1.2
RELEASE_1.1.1
RELEASE_1.1.0
RELEASE_1.0.9
RELEASE_1.0.8
RELEASE_1.0.7
RELEASE_1.0.6
RELEASE_1.0.5
RELEASE_1.0.4
RELEASE_1.0.3
RELEASE_1.0.2
RELEASE_1.0.1
RELEASE_1.0.0
RELEASE_0.9.9
RELEASE_0.9.8
RELEASE_0.9.7
RELEASE_0.9.6
RELEASE_0.9.5
RELEASE_0.9.4
RELEASE_0.9.3
RELEASE_0.9.2
RELEASE_0.9.1
RELEASE_0.9.0
RELEASE_0.8.9
RELEASE_0.8.8
RELEASE_0.8.7
RELEASE_0.8.6
RELEASE_0.8.5
RELEASE_0.8.4
RELEASE_0.8.3
RELEASE_0.8.2
RELEASE_0.8.1
RELEASE_0.8.0
RELEASE_0.7.9
RELEASE_0.7.8
RELEASE_0.7.7
RELEASE_0.7.6
RELEASE_0.7.5
RELEASE_0.7.4
RELEASE_0.7.3
RELEASE_0.7.2
RELEASE_0.7.1
RELEASE_0.7.0
RELEASE_0.6.9
RELEASE_0.6.8
RELEASE_0.6.7
RELEASE_0.6.6
RELEASE_0.6.5
RELEASE_0.6.4
RELEASE_0.6.3
RELEASE_0.6.2
RELEASE_0.6.1
RELEASE_0.6.0
RELEASE_0.5.9
RELEASE_0.5.8
RELEASE_0.5.7
RELEASE_0.5.6
RELEASE_0.5.5
RELEASE_0.5.4
RELEASE_0.5.3
RELEASE_0.5.2
RELEASE_0.5.1
RELEASE_0.5.0
RELEASE_0.4.9
RELEASE_0.4.8
RELEASE_0.4.7
RELEASE_0.4.6
RELEASE_0.4.5
RELEASE_0.4.4
RELEASE_0.4.3
RELEASE_0.4.2
RELEASE_0.4.1
RELEASE_0.4.0
RELEASE_0.3.9
RELEASE_0.3.8
RELEASE_0.3.7
RELEASE_0.3.6
RELEASE_0.3.5
RELEASE_0.3.4
RELEASE_0.3.3
RELEASE_0.3.2
RELEASE_0.3.1
RELEASE_0.3.0
RELEASE_0.2.9
RELEASE_0.2.8
RELEASE_0.2.7
RELEASE_0.2.6
RELEASE_0.2.5
RELEASE_0.2.4
RELEASE_0.2.3
RELEASE_0.2.2
RELEASE_0.2.1
RELEASE_0.2.0
RELEASE_0.1.9
RELEASE_0.1.8
RELEASE_0.1.7
RELEASE_0.1.6
RELEASE_0.1.5
RELEASE_0.1.4
RELEASE_0.1.3
RELEASE_0.1.2
RELEASE_0.1.1
RELEASE_0.1.0
RELEASE_0.0.9
RELEASE_0.0.8
RELEASE_0.0.7
RELEASE_0.0.6
RELEASE_0.0.5
RELEASE_0.0.4
RELEASE_0.0.3
RELEASE_0.0.2
RELEASE_0.0.1
Labels
Clear labels   
awaiting user feedback

   
bug

   
cmake

   
cmake

   
docs

   
feature request

   
in progress

   
long term

   
medium term

   
medium term

   
pull-request

Mirrored from GitHub Pull Request

  
question

   
question

   
ready to close

   
short term

   
under investigation

   
wontfix
No labels
awaiting user feedback
bug
cmake
cmake
docs
feature request
in progress
long term
medium term
medium term
pull-request
question
question
ready to close
short term
under investigation
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/libxlsxwriter#143
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?