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

Cleanup of cmake based projects #1621

Merged
merged 4 commits into from Jan 8, 2017
Merged

Cleanup of cmake based projects #1621

merged 4 commits into from Jan 8, 2017

Conversation

starius
Copy link
Member

@starius starius commented Jan 1, 2017

Use CMake wrapper in all but native packages.
Delete cmake arguments specifying build type (static, shared).
Move some of common cmake variables to the toolchain file.
Refactor definitions of some variables (less repeats, more code reusing).

@pavelvat
Copy link
Contributor

pavelvat commented Jan 1, 2017

'$(TARGET)-cmake'

Quotes are not needed. Because $(TARGET) is not a path.

@starius
Copy link
Member Author

starius commented Jan 1, 2017

Quotes are not needed.

This is a good point. Do we expect new targets with space in name?
In most cases in our code $(TARGET) is quoted. Maybe we should remove the quotes.

@starius
Copy link
Member Author

starius commented Jan 1, 2017

We quote even '$(JOBS)'. I think, unquoting '$(JOBS)', $(TARGET) and so on should be done, but it is out of scope of this pull request.

@starius
Copy link
Member Author

starius commented Jan 1, 2017

Currently it fails to build. All vars are OFF in all targets:

set(BUILD_SHARED_LIBS OFF)
set(BUILD_STATIC_LIBS OFF)
set(BUILD_SHARED OFF)
set(BUILD_STATIC OFF)

@starius
Copy link
Member Author

starius commented Jan 2, 2017

The problem seems to be introduced by changes in Makefile in 48b3bdb
Why doesn't it work? The manual says

All variables that appear within the variable-assignment are evaluated within the context of the target: thus, any previously-defined target-specific variable values will be in effect.

so it should work, but it doesn't.

Anyway this change is out of scope of this pull request (cleanup of cmake based projects), so I removed it.

@starius
Copy link
Member Author

starius commented Jan 2, 2017

Now it fails, because set(BUILD_STATIC ON) in a toolchain file works in different way than -DBUILD_STATIC=ON in cmake command line. The former does not override option(BUILD_STATIC "Build a static library" OFF).

set(BUILD_STATIC ON CACHE BOOL "BUILD_STATIC" FORCE) works. I am starting a new build.

@starius
Copy link
Member Author

starius commented Jan 3, 2017

Now pcl fails.
It was built with CMAKE_BUILD_TYPE=RelWithDebInfo.

From toolchain file:

set(CMAKE_BUILD_TYPE Release CACHE STRING "Debug|Release|RelWithDebInfo|MinSizeRel")

I will try to add FORCE there.

Native binaries (libmysqlclient, vtk, vtk6) still build with `cmake`
instead of the cmake wrapper. Note that even in these cases MXE's cmake
is used (not system cmake), because $(PREFIX)/$(BUILD)/bin is added to
PATH by Makefile and MXE's cmake binary exists there.
Only BUILD_SHARED_LIBS was there before.
@starius starius changed the title [WIP] Cleanup of cmake based projects Cleanup of cmake based projects Jan 8, 2017
@starius
Copy link
Member Author

starius commented Jan 8, 2017

Now it builds correctly. Lists of files produced by all packages are the same.

@starius starius merged commit d292301 into mxe:master Jan 8, 2017
@starius starius deleted the cmake-cleanup branch January 8, 2017 02:18
@tonytheodore
Copy link
Member

Why doesn't it work? The manual says

All variables that appear within the variable-assignment are evaluated within the context of the target: thus, any previously-defined target-specific variable values will be in effect.

so it should work, but it doesn't.

The variables need an extra $ to defer evaluation i.e:

build-only-$(1)_$(3): LIB_SUFFIX = $(if $$(BUILD_SHARED),dll,a)

See references to SOURCE_DIR and BUILD_DIR

@starius
Copy link
Member Author

starius commented Jan 20, 2017

The variables need an extra $ to defer evaluation

I tried to use double $ , but it resulted in mxe-conf.cmake with both BUILD_SHARED_LIBS and BUILD_STATIC_LIBS set to ON (target was i686-w64-mingw32.static).

@tonytheodore
Copy link
Member

I see, the function needs to be deferred also:

@@ -564,8 +564,8 @@ build-only-$(1)_$(3): TEST_FILE  = $($(1)_TEST_FILE)
 build-only-$(1)_$(3): CMAKE_RUNRESULT_FILE = $(PREFIX)/share/cmake/modules/TryRunResults.cmake
 build-only-$(1)_$(3): CMAKE_TOOLCHAIN_FILE = $(PREFIX)/$(3)/share/cmake/mxe-conf.cmake
 build-only-$(1)_$(3): CMAKE_TOOLCHAIN_DIR  = $(PREFIX)/$(3)/share/cmake/mxe-conf.d
-build-only-$(1)_$(3): CMAKE_STATIC_BOOL = $(if $(findstring shared,$(3)),OFF,ON)
-build-only-$(1)_$(3): CMAKE_SHARED_BOOL = $(if $(findstring shared,$(3)),ON,OFF)
+build-only-$(1)_$(3): CMAKE_STATIC_BOOL = $$(if $$(BUILD_STATIC),ON,OFF)
+build-only-$(1)_$(3): CMAKE_SHARED_BOOL = $$(if $$(BUILD_SHARED),ON,OFF)
 build-only-$(1)_$(3):
        $(if $(value $(call LOOKUP_PKG_RULE,$(1),BUILD,$(3))),
            uname -a

src/mxe-conf.mk Outdated
echo 'set(BUILD_SHARED_LIBS $(CMAKE_SHARED_BOOL) CACHE BOOL "BUILD_SHARED_LIBS" FORCE)'; \
echo 'set(BUILD_STATIC_LIBS $(CMAKE_STATIC_BOOL) CACHE BOOL "BUILD_STATIC_LIBS" FORCE)'; \
echo 'set(BUILD_SHARED $(CMAKE_SHARED_BOOL) CACHE BOOL "BUILD_SHARED" FORCE)'; \
echo 'set(BUILD_STATIC $(CMAKE_STATIC_BOOL) CACHE BOOL "BUILD_STATIC" FORCE)'; \
Copy link
Contributor

Choose a reason for hiding this comment

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

This behaviour simplifies building MXE itself but complicates its usage for developing of own libraries by end users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on this, please? Do you need to redefine BUILD_SHARED_LIBS in a CMakeLists.txt of a library?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on this, please?

Yes, sure.

  1. There are situations when you need to prepare static library for internal usage in your project despite the fact that all third-party libraries are shared ones.
  2. There are situations when you need to prepare shared library which is statically linked with third-party libraries.
  3. There are (more rare) situations when you need to compile shared and static versions of your libraries simultaneously.
  4. BUILD_SHARED_LIBS and BUILD_STATIC_LIBS options are commonly used for switching between different types of build. From my personal experience situations when specific options (like FREEGLUT_BUILD_STATIC_LIBS and FREEGLUT_BUILD_SHARED_LIBS) are used for this purpose are very rare.

So forcing one type of build in cmake config may be inconvenient for end users. But of course they may fix it themself...

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.

None yet

4 participants