Conversation
Croydon
left a comment
Croydon
left a comment
There was a problem hiding this comment.
There are a lot of changes that are unrelated to increase the minimum CMake version, and many of those changes are non-obvious, that require much more explanation of their motivation and effects IMHO.
buildClangCl.bat
Outdated
| @@ -0,0 +1,5 @@ | |||
| mkdir CMakeBuildClangCl | |||
There was a problem hiding this comment.
I think we should stop adding more of those bat files. In times where CMake is well integrated in editors, we really shouldn't need them. These are highly unflexible anyway
There was a problem hiding this comment.
Ok, I'll remove it, shall I remove the others too?
There was a problem hiding this comment.
Just my make my role here clear: I contributed to winflexbison before, but I have no official say here in any way. So I am stating my own personal opinions here.
I am not sure if someone still relies on the existing bat files, but since the development of winflexbison is mostly dead, it can't affect too many people for sure, if we would remove all of them. But maybe this is something for another pull request.
There was a problem hiding this comment.
Another pull request it is... concerning cmake I'll let @Croydon decide, for the rest... stay compatible :-)
|
Can you please rebase and split the clang parts into a separate PR that would fix #103? |
Febbe
force-pushed
the
master
branch
2 times, most recently
from
c2cda58 to
1397107
Compare
Febbe
force-pushed
the
master
branch
3 times, most recently
from
96f5524 to
a95d9f7
Compare
|
Is there a reason to prefer 3.21 over 3.15/3.10 (after all it is a minimal version)? |
|
I have no strong feelings about that feature, but FYI the original comparison was bug prone and it's now recommended to use this variable instead. 3.10 is already deprecated in newer versions and the support will be removed soon. Therefore I would not stay there. As you soon have to bump the version again. I checked the last two Ubuntu LTS releases, both already ship a higher cmake version so most of the people won't have an issue with 3.21. Beside the fact that one can always install the newest cmake version manually. |
|
No problem with the version, mainly wanted to ask (we just add that info to the commit log). RHEL7 has only cmake 2.x, RHEL8 already 3.26; only the oldest still supported current Debian (Bullseye) has 3.18 - but we build for Windows here, so if we don't expect Bison/Flex upstream to additional include cmake support then we can use 3.21... and even if: those projects won't leave autotools as the preferred way so "does only work that way, not with cmake" is no issue either. Concerning this PR: is -D_DEBUG "lost"? On purpose? |
Oh right, a new cmake version is no issue on windows. You always have to install it manually.
Yes on purpose as stated in the review comment. |
|
@Febbe wrote
as we have a fixed cmake dependency - that's a good idea! Would you like to inspect that (either within this or possibly with a next PR)? |
| endif () | ||
| if (MSVC) | ||
| add_compile_options("/source-charset:utf-8") | ||
| option(WINFLEXBISON_USE_STATIC_RUNTIME "Set ON to change /MD(DLL) to /MT(static)" Off) |
There was a problem hiding this comment.
This multiple option for WINFLEXBISON_USE_STATIC_RUNTIME will be a nightmare for maintenance, preferable to consolidate them into a single variable before setting it
There was a problem hiding this comment.
And also, don't expect anyone to dig through the PRs, please add a comments around the need for WINFLEXBISON_USE_STATIC_RUNTIME, I don't really know why at a glance, like its to prevent name clash with other code base that pull this in as a dependency if they use USE_STATIC_RUNTIME but we use this name previously so older scripts we missed will still work, etc.
| set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/bin") | ||
| set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/bin") | ||
| set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELWITHDEBINFO "${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/bin") |
There was a problem hiding this comment.
I haven't experimented this myself, why separate them, is the single CMAKE_RUNTIME_OUTPUT_DIRECTORY not working for some reason?
There was a problem hiding this comment.
There is no reason, it was separated before I refactored it and was too tired to see that. Good catch.
Suggest more modern cmake commands. Co-authored-by: jonnysoe <84360198+jonnysoe@users.noreply.github.com>
Co-authored-by: jonnysoe <84360198+jonnysoe@users.noreply.github.com>
4 participants
Fixes #98Previously a PR to fix #98, but was superseded by #101 and #104.
Only contains changes the other commits haven't done.