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

Update CMake scripts #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update CMake scripts #160

wants to merge 1 commit into from

Conversation

drizt
Copy link
Contributor

@drizt drizt commented Jan 17, 2018

No description provided.

add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${RESOURCE}.o
COMMAND ${CMAKE_RC_COMPILER} -I${CMAKE_CURRENT_SOURCE_DIR} -i${CMAKE_CURRENT_SOURCE_DIR}/${RESOURCE}.rc
-o ${CMAKE_CURRENT_BINARY_DIR}/${RESOURCE}.o)
set(LANGUAGES
Copy link
Owner

Choose a reason for hiding this comment

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

This is yet another place where languages list has to be maintained (and manually). Is there no way in CMake to look for all available *.po files instead?

Copy link

Choose a reason for hiding this comment

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

You could use file(GLOB VARIABLE ${ROOT_DIR}/translations/*.po CONFIGURE_DEPENDS) (https://cmake.org/cmake/help/v3.12/command/file.html#glob), but it was new in CMake 3.12 so you'd have to rause your cmake_minimum_required quite a bit. Before that there was no way to get CMake to re-configure if the glob results change. I'm not sure if 2.8 (which is your current minimum) even has file(GLOB) at all.

Copy link
Owner

Choose a reason for hiding this comment

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

@puetzk Thanks! How problematic is raising CMake version? I think not much on Windows where distro packaging doesn't enter the picture… AppVeyor, which I care about for CI, already includes 3.13.

Copy link
Owner

Choose a reason for hiding this comment

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

@puetzk However not using CMake myself, I don't really know how to translate your advice into actual code; a patch/PR would be tremendously helpful.

Copy link
Contributor

@Youw Youw Apr 28, 2019

Choose a reason for hiding this comment

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

file(GLOB) available at least since CMake 2.6, so no raising is required.

Copy link

Choose a reason for hiding this comment

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

But that's missing CONFIGURE_DEPENDS (which is what was new in 3.12), so it won't reconfigure properly if the filesystem changes.

Copy link
Contributor

@Youw Youw Apr 29, 2019

Choose a reason for hiding this comment

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

So if and only if:

  • already run cmake command (configure build/generate build scripts);
  • after that you add some new *.po file(s);
  • and run cmake --build (or make, if this is mingw build, or build in Visual Studio);

In that case the build won't include new files.

If you run cmake (configure) command manually (which you normally do, if you have CI, or smth) - everything works.
If you remove existing .po file - build reasonably fails, until you run cmake.
If you do a clean build - everything works by default anyway.

Don't think this small feature is worth considering raising minimal required cmake version, specially for build system that is not officially supported.

I've seen two approaches, which I'd support widely:

  1. Support cmake 2.8 and higher - great if you want to have(already have) a wide range of supported platforms/users;
  2. Support only latest cmake release - great for development, specially if you control all the build environments;

Anything in between sounds like not a great compromise.

@@ -0,0 +1,7 @@
/*
MinGW can't compile multiple .rc files. Only single.
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps a better fix, simpler for MinGW, would be to include translations.rc from winsparkle.rc then?

@@ -0,0 +1,91 @@
/* expat_config.h.in. Generated from configure.in by autoheader. */
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need this file at all, when it wasn't previously needed? This doesn't seem right, surely this won't be accurate for all possible compilers.

add_library(${PROJECT_NAME} SHARED ${SOURCES} $<TARGET_OBJECTS:wxWidgets> $<TARGET_OBJECTS:expat>)

target_link_libraries(${PROJECT_NAME} wininet version rpcrt4 comctl32)

#set_target_properties(${PROJECT_NAME} PROPERTIES
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't include commented out code — either it is needed, in which case it shouldn't be commented out, or it isn't, in which case it should be there at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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

Successfully merging this pull request may close these issues.

4 participants