msgpack / msgpack-c

MessagePack implementation for C and C++ / msgpack.org[C/C++]
Other
3.03k stars 883 forks source link

msgpack-c 6.0.1 generates invalid pkg-config file #1128

Closed fumoboy007 closed 4 months ago

fumoboy007 commented 4 months ago

Describe the bug msgpack-c 6.0.1 generates an invalid msgpack-c.pc pkg-config file. The libdir and includedir variables are missing the prefix:

prefix=/opt/homebrew/Cellar/msgpack/6.0.1
exec_prefix=/opt/homebrew/Cellar/msgpack/6.0.1
libdir=lib
includedir=include

Name: MessagePack
Description: Binary-based efficient object serialization library
Version: 6.0.1
Libs: -L${libdir} -lmsgpack-c
Cflags: -I${includedir}

To Reproduce Just build/install the project with default settings.

Expected behavior msgpack-c 6.0.0 generates a valid msgpack-c.pc:

prefix=/opt/homebrew/Cellar/msgpack/6.0.0
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: MessagePack
Description: Binary-based efficient object serialization library
Version: 6.0.0
Libs: -L${libdir} -lmsgpack-c
Cflags: -I${includedir}

Notice that libdir and includedir correctly include the prefix.

fumoboy007 commented 4 months ago

d1af4a3cdb4beb60490c5548f677928340684db0 is the culprit.

The bug was already fixed by 68cc50a3de991e5b534ac628b7f9d1a0475e5bfc and 4e027b72de6e5a7f216482bdeea1ddabe53474cf. We just need to release a new version including those two commits.

Perhaps we should also consider removing the 6.0.1 release to avoid confusion.

redboltz commented 4 months ago

Could you create a pull request that removes https://github.com/msgpack/msgpack-c/commit/d1af4a3cdb4beb60490c5548f677928340684db0 effect ?

Then I will check it and merge it. Then I will release the merged c_master as 6.0.2 with CHANGELOG.

fumoboy007 commented 4 months ago

@redboltz c_master already has the fix that I mentioned, so we can just create 6.0.2 from the current c_master. Or did I misunderstand you?

redboltz commented 4 months ago

Ok, I understand. I will release 6.0.2 soon.

redboltz commented 4 months ago

I noticed that #1124 and #1125 are related issues. Now I am confirming that I should merge #1125 and close #1124. After this issue would be solved, I will release.

@fumoboy007, could you try that #1125 works as you expected ?

fumoboy007 commented 4 months ago

Ah, yes. That is a separate issue but also related, so would be good to include in 6.0.2.

I confirmed that #1125 works as expected. It is a small change, so I think we should merge this one and release 6.0.2.

1124 is a more foundational change that may reduce complexity in the long term though I am not sure whether the current pull request is bug-free. It’s probably too risky of a change for 6.0.2 but we could keep it open and let @saper and @otegami decide on what to do about it later.

fumoboy007 commented 4 months ago

By the way, I just noticed that tag c-6.0.2 already exists!

redboltz commented 4 months ago

By the way, I just noticed that tag c-6.0.2 already exists!

Yes, I was working to release c-6.0.2 just before. Current c-6.0.2 is incorrect and not released. I will force update later. However, it would make confusion, so I removed it just now.

redboltz commented 4 months ago

6.0.2 has been pre released. https://github.com/msgpack/msgpack-c/releases/tag/c-6.0.2

Could you check this ?

After you checking, I will release it.

fumoboy007 commented 4 months ago

Confirmed that version 6.0.2 works as expected. Thanks!

redboltz commented 4 months ago

Now 6.0.2 has been released.