search

radareorg / sdb

Simple and fast string based key-value database with support for arrays and json
https://www.radare.org/
MIT License
218 stars 62 forks source link

create pkg-config file in meson and make tests runnable from meson #158

Closed ret2libc closed 5 years ago

ret2libc commented 6 years ago

So I have considered this fact and the reasons I went ahead with this anyway were:

  1. radare2 does the same
  2. without pkg-config you are forced to have all those "../../../" all around CFLAGS/LDFLAGS
  3. make builds in the src dir, so you can test only one version of sdb at a given time, otherwise you have to recompile everything. Meson can build in directories with different names, so you can't assume that all the stuff will be in ../../build/
  4. with pkg-config you are not forced to install libs/headers system-wide but it's enough to correctly set PKG_CONFIG_PATH/LD_LIBRARY_PATH/PATH like we do for radare2 travis and you can easily switch between different installation types
  5. you test what the final user will actually use.

I understand it is handy to just do make; make tests but without pkg-config it becomes a big mess and very much "path dependent".

ret2libc commented 6 years ago

For example, I just found out that the acr+make install wasn't working well because it didn't install some required headers. This was not caught by tests before because they were just using the source directory.

radare commented 6 years ago

which required headers? sdb builds fine in travis without installing anything

On 21 Sep 2018, at 09:46, Riccardo Schirone notifications@github.com wrote:

For example, I just found out that the acr+make install wasn't working well because it didn't install some required headers. This was not caught by tests before because they were just using the source directory.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/radare/sdb/pull/158#issuecomment-423445324, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-lrqywoyom1stAMWSaw0Cku6UXmbnks5udJlrgaJpZM4Wy05O.

ret2libc commented 6 years ago

As said, it was running well because you are not installing it. If a user would try to install sdb and use it from the system, it wouldn't work, because src/sdbht.h was not installed on the system (see the changes in this PR)

ret2libc commented 6 years ago

@radare ping

radare commented 6 years ago

Busy at work and irl. Had no time to test and im not convinced on having pkgcondig as dependency at compile time

On 24 Sep 2018, at 14:16, Riccardo Schirone notifications@github.com wrote:

@radare ping

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

radare commented 6 years ago

Also theres the problem of dynamic an dtstic linking

On 24 Sep 2018, at 14:16, Riccardo Schirone notifications@github.com wrote:

@radare ping

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ret2libc commented 6 years ago

What problem of static/dynamic linking?

radare commented 6 years ago

I prefer static linking because this makes sdb self contained and give no room to problems like envvars or different versions of libs mainly because libsdb is not versioned at all and will cause major on problems on many people if they have multiple versions installed. Thats why sdb is embedded inside r2 and why static linking is prefered in the current makefile.

Btw pkgconfig doesnt works much well when having static and dynamic options of the same lib. Its usually better to have two separate pc files instead. Which sucks but cant think of a better solution

On 24 Sep 2018, at 14:58, Riccardo Schirone notifications@github.com wrote:

What problem of static/dynamic linking?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ret2libc commented 6 years ago

because libsdb is not versioned at all and will cause major on problems on many people if they have multiple versions installed

That's why in my previous commit about meson I've versioned the dynamic library (https://github.com/radare/sdb/blob/master/meson.build#L61)

Btw pkgconfig doesnt works much well when having static and dynamic options of the same lib. Its usually better to have two separate pc files instead. Which sucks but cant think of a better solution

Again, I don't think these problems apply with sdb, since the pkg-config file is very simple and sdb doesn't have dependencies. Indeed just a: cc --static $(pkg-config --cflags --static sdb) -o test_hash ./test_hash.c $(pkg-config --libs --static sdb) works well.

ret2libc commented 6 years ago

im not convinced on having pkgcondig as dependency at compile time

I see some reasons why this is not really a problem.

  1. sdb is used only in radare2 AFAIK and radare2 already requires pkg-config
  2. pkg-config can be made optional (though that doesn't prevent tests in travis from requiring it)
ret2libc commented 6 years ago

Moreover, if it becomes really necessary, we can create a env.sh script similar to the r2 one to switch between different "install" directories.

ret2libc commented 6 years ago

@radare ping

ret2libc commented 6 years ago

Ok I'm trying to keep tests runnable without installing sdb.

ret2libc commented 6 years ago

ok @radare please consider merging this since it allows to run tests from a not-installed sdb both from makefile and meson (with the BASEDIR var)

radare commented 5 years ago

what about having sdb.mk or deps.mk and put all the logic of defining flags and path to SDB in only one place? just include it from the shellscripts and the makefiles