Skip to content

feat(cmake): make BUILD_SEPARATE_LIBS work with BUILD_MERGED_PLUGINS #260

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

feat(cmake): make BUILD_SEPARATE_LIBS work with BUILD_MERGED_PLUGINS #260

wants to merge 8 commits into from

Conversation

lotem
Copy link
Member

@lotem lotem commented Mar 7, 2019 •
edited
Loading

create separate libraries by module, and an optional rime-plugins library.

@lotem lotem requested a review from a team March 7, 2019 12:34
@lotem
Copy link
Member Author

lotem commented Mar 7, 2019

@hchunhui Please have a look.
The *-objs target doesn't work well in macOS build, I couldn't create individual dylibs for plugins, and had to use a work-around (adding an empty .cc file) to create the merged librime-plugins.dylib.

@lotem lotem mentioned this pull request Mar 7, 2019
Merged
@hchunhui
Copy link
Contributor

hchunhui commented Mar 7, 2019

@lotem I can't reproduce the issue in Linux (CMake 3.13.4).
I have passed four build modes (switch on/off two flags), after I delete plugins/plugins.c.

So I think it is a macOS specific issue. Maybe we should keep the workaround.

@lotem
Copy link
Member Author

lotem commented Mar 7, 2019 •
edited
Loading

@hchunhui That's true. I also tested in Linux, different results.

Another difference is: in Linux there's no need to target_link_libraries(a-plugin rime rime-dict ...)
but it is a link error if rime is not in its deps in macOS build (not sure if it's because the BUILD_STATIC=ON option or because of different CMake implemetation (i saw many .sh scripts created by CMake on macOS, but was Makefiles in case of Linux)).

because of this, I have to repeat ${rime_library} ... in both target_link_libraries() and add_dependencies(). the latter alone works for Linux build.

hchunhui
hchunhui reviewed Mar 7, 2019
View reviewed changes
src/CMakeLists.txt
install(TARGETS rime-dict DESTINATION ${LIB_INSTALL_DIR})

add_library(rime-gears ${rime_gears_src})
target_link_libraries(rime-gears
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target_link_libraries(rime-gears
target_link_libraries(rime-gears ${rime_gears_deps})

There is no need to link ${rime_library}, ${rime_gears_deps}, ${rime_dict_library}.

I have tested it in Linux, and it should work in macOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

No this doesn't work for the macOS build.
Errors were cannot find symbol in rime libraries it depends on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the solution is

target_link_libraries(rime-gears ${rime_gears_deps})
IF(APPLE)
  SET_TARGET_PROPERTIES(rime-gears PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
ENDIF(APPLE)

References:
[1] https://stackoverflow.com/questions/42722759/portable-plugins-with-cmake
[2] https://stackoverflow.com/questions/36662920/xcode-clang-link-build-dynamic-framework-or-dylib-not-embed-dependencies

hchunhui
hchunhui reviewed Mar 7, 2019
View reviewed changes
src/CMakeLists.txt Outdated
${rime_gears_deps}
${rime_library}
${rime_dict_library})
add_dependencies(rime-gears
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I'm testing. Seems to work without add_dependencies(). As all folks say.

hchunhui
hchunhui reviewed Mar 7, 2019
View reviewed changes
src/CMakeLists.txt
if(XCODE_VERSION)
set_target_properties(rime-gears PROPERTIES INSTALL_NAME_DIR "@rpath")
endif()
install(TARGETS rime-gears DESTINATION ${LIB_INSTALL_DIR})

add_library(rime-levers ${rime_levers_src})
target_link_libraries(rime-levers
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

hchunhui
hchunhui reviewed Mar 7, 2019
View reviewed changes
src/CMakeLists.txt
add_library(rime-plugins
${rime_plugins_src}
${rime_plugins_objs})
target_link_libraries(rime-plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Just link ${rime_plugins_deps}

hchunhui
hchunhui reviewed Mar 7, 2019
View reviewed changes
tools/CMakeLists.txt

set(rime_patch_src "rime_patch.cc")
add_executable(rime_patch ${rime_patch_src})
target_link_libraries(rime_patch ${rime_library} ${rime_gears_library})
add_dependencies(rime_patch ${rime_library} ${rime_gears_library})
target_link_libraries(rime_patch
Copy link
Contributor

Choose a reason for hiding this comment

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

My test shows rime_patch depends on ${rime_dict_library}

@hchunhui
Copy link
Contributor

hchunhui commented Mar 7, 2019

@hchunhui That's true. I also tested in Linux, different results.

Another difference is: in Linux there's no need to target_link_libraries(a-plugin rime rime-dict ...)
but it is a link error if rime is not in its deps in macOS build (not sure if it's because the BUILD_STATIC=ON option or because of different CMake implemetation (i saw many .sh scripts created by CMake on macOS, but was Makefiles in case of Linux)).

because of this, I have to repeat ${rime_library} ... in both target_link_libraries() and add_dependencies(). the latter alone works for Linux build.

I see. The reason is that you use static build.
I can fix the issue.

@hchunhui
Copy link
Contributor

hchunhui commented Mar 7, 2019

I need some time to recall the solution.
Maybe I will commit tomorrow.

@lotem
Copy link
Member Author

lotem commented Mar 7, 2019 •
edited
Loading

No worries. I'll working on it.
I'll do some basic test on Windows too.

@lotem
Copy link
Member Author

lotem commented Mar 7, 2019 •
edited
Loading

By the way, while experimenting with the librime-legacy plugin, I'm also working to change the for-Linux Makefile as follows:

  • add a target merged-plugins that creates a release build that merges any existing plugins/*
  • the default release target set BUILD_MERGED_PLUGINS=OFF, with two considerations:
    1. releaser (to linux distros) of the librime package doesn't have to change the build rules to avoid including any plugin code in the binary package;
    2. every plugin can be built as a separate binary and make a package of its own; build instructions stay simple:
      cd librime; git checkout ... plugins/x; make && make install which installs both librime.so and x.so

In CMakeLists.txt, (and in contrast to building for release purposes,) BUILD_MERGED_PLUGINS is by default ON so it's easy to integrate plugins in local build.

@lotem
Copy link
Member Author

lotem commented Mar 8, 2019

Merged.

@lotem lotem closed this Mar 8, 2019
@lotem
Copy link
Member Author

lotem commented Mar 8, 2019

FYI
I updated the sample plugin and wrote some notes in sample/README.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hchunhui hchunhui hchunhui left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lotem @hchunhui