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

msgpack-cxx: make headeronly and boost optional #4648

Merged
merged 16 commits into from
Jul 14, 2024

Conversation

Chi-EEE
Copy link
Contributor

@Chi-EEE Chi-EEE commented Jul 13, 2024

No description provided.

@Chi-EEE Chi-EEE marked this pull request as ready for review July 13, 2024 14:43
@Chi-EEE Chi-EEE changed the title msgpack-cxx: make boost optional msgpack-cxx: make headeronly and boost optional Jul 13, 2024
table.insert(configs, "-DMSGPACK_USE_BOOST=OFF")
end
import("package.tools.cmake").install(package, configs)
os.cp("include", package:installdir())
Copy link
Member

Choose a reason for hiding this comment

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

Whenever possible, use cmake even if it is headeronly, as cmake install may also install an additional xxx.cmake import file that will allow other cmake packages to find it better.

Copy link
Contributor Author

@Chi-EEE Chi-EEE Jul 13, 2024

Choose a reason for hiding this comment

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

I get package(msgpack-cxx): links not found! errors when I use cmake install for the checks, the library does not generate a .lib file so cmake is not needed

Copy link
Member

Choose a reason for hiding this comment

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

you need only add set_kind("library", {headeronly = true})

@Chi-EEE Chi-EEE marked this pull request as draft July 13, 2024 18:05
@Chi-EEE Chi-EEE marked this pull request as ready for review July 13, 2024 18:14
@@ -1,5 +1,5 @@
package("msgpack-cxx")

set_kind("library", {headeronly = true})
Copy link
Member

Choose a reason for hiding this comment

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

we should set it dynamicaly in on_load. use package:config("header_only")

Copy link
Contributor Author

@Chi-EEE Chi-EEE Jul 14, 2024

Choose a reason for hiding this comment

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

setting it dynamically in on_load would not work as the cmake file does not generate a .lib file if it is not linked with boost, it only generates a lib file when linking with boost

Copy link
Member

Choose a reason for hiding this comment

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

package:config("header_only") should be consistent with the state of set_kind("library", {headeronly = true}), otherwise there is no point in adding header_only config.

If the library is always headeronly, we shouldn't add add_configs("header_only")

@waruqi
Copy link
Member

waruqi commented Jul 14, 2024

I tried it and even with cmake.install and boost enabled, it's still a headeronly library, so why not use cmake?

It seems works fine.

checking for xmake::msgpack-cxx ... msgpack-cxx 6.1.1
{
  license = "BSL-1.0",
  sysincludedirs = {
    "/Users/ruki/.xmake/packages/m/msgpack-cxx/6.1.1/cdfe231ae33d45ab98e4c0c70e3894d3/include ,
  },
  version = "6.1.1"
}

patching /Users/ruki/.xmake/packages/m/msgpack-cxx/6.1.1/cdfe231ae33d45ab98e4c0c70e3894d3/lib/p
kgconfig/msgpack-cxx.pc ..
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-c -Qunused-arguments -target x86_64-apple-macos14.0 -isysroot /Applications/Xcode.app/Contents
/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -std=c++17 -isystem /Users/r
uki/.xmake/packages/m/msgpack-cxx/6.1.1/cdfe231ae33d45ab98e4c0c70e3894d3/include -isystem /User
s/ruki/.xmake/packages/b/boost/1.85.0/ea54d63440cc46f2befc6c2c911bdf9f/include -o /var/folders/
32/w9cz0y_14hs19lkbs6v6_fm80000gn/T/.xmake501/240714/_21CB8C8128B444108E63739F6CA44830.o /var/f
olders/32/w9cz0y_14hs19lkbs6v6_fm80000gn/T/.xmake501/240714/_407F0AF53F7C47FD94352FD56BBEED07.c
pp
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang+
+ -o /var/folders/32/w9cz0y_14hs19lkbs6v6_fm80000gn/T/.xmake501/240714/_21CB8C8128B444108E63739
F6CA44830.b /var/folders/32/w9cz0y_14hs19lkbs6v6_fm80000gn/T/.xmake501/240714/_21CB8C8128B44410
8E63739F6CA44830.o -target x86_64-apple-macos14.0 -isysroot /Applications/Xcode.app/Contents/De
veloper/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -lz -L/Users/ruki/.xmake/packag
es/b/boost/1.85.0/ea54d63440cc46f2befc6c2c911bdf9f/lib -lboost_atomic-mt -lboost_filesystem-mt
> checking for c++ includes(msgpack.hpp)
> checking for c++ links(boost_atomic-mt, boost_filesystem-mt)
> checking for c++ snippet(            #inc)
  => install msgpack-cxx 6.1.1 .. ok

@waruqi waruqi merged commit 7169f79 into xmake-io:dev Jul 14, 2024
65 checks passed
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

2 participants