Not sure what to do about: https://github.com/mesonbuild/wrapdb/pull/179/checks?check_run_id=3805066460#step:5:25

This is the same problem as in the xtensor PR. Meson in git master no longer reports that (fatal) warning on Debian. Windows would still be broken, but there is discussion on fixing it so that may be fixable by adding an option -Dpython.install_platlib or similar. Which would then be handled by the sanity check script directly.

Good to know that I haven't done anything wrong!

Maybe in the meantime we can add in ci_config.json a new field to skip --meson-fatal-warnings?

@xclaesse do you mean to special case hse in there? Or does something have to be done to the ci_config.json format?

I mean you could add in ci_config.json something like:

  "hse": {
    "ignore-meson-warning": true
  }

And in sanity_checks.py you can check that field and skip the --fatal-meson-warnings arg.

@eli-schwartz if you think that is a good idea, I can go ahead and create a PR for that.

TBH I'm not too fond of the idea, because it fundamentally amounts to "meson is broken here, let's add an option to just ignore that" and the solution really is just to wait for meson 0.60 -- which is going into rc1 on Sunday.

I have a feeling that the CI for HSE will fail after 0.60.0 because we explicitly set -std=c++14 in one of our built-in subprojects because otherwise Meson takes the cpp_std value of the parent project which in my opinion is just wrong behavior, especially if cpp isn't even one of the languages of the parent project. I did see precedent for setting options in ci_config.json however, so after I confirm the failure, then I will add the option to disable building that specific subproject.

Should be fixed if you rebase on master I believe

This failure makes no sense to me: https://github.com/mesonbuild/wrapdb/pull/179/checks?check_run_id=3884806754#step:5:116.

Where did it grab lz4 1.9.3 from? I specifically have 1.9.2 in my packagefile directory. For right now, I don't use the lz4 in the wrapdb.

@tristan957 lz4 1.9.3 is in wrapdb, it is a nice (but maybe not obvious) property of how wrapdb works, wraps fallbacks to other wraps.

Right, but I don't use the WrapDB variant because it doesn't support 1.9.2.

Wraps from within a subproject cannot be promoted to override a wrap that is in the superproject.

What is wrong with 1.9.3 ?

Right, but I don't use the WrapDB variant because it doesn't support 1.9.2.

Wraps from wrapdb takes priority over wraps from hse, because in this CI wrapdb is the main project and hse is a subproject.

Any reason you cannot make it work with lz4 from wrapdb?

Well we use a specific define to inline lz4 that isn't exposed in the WrapDB wrap and I can't pass c_args through to a subproject. Not to mention that my project isn't even tested against 1.9.3.

How does this define work in conjunction with system built lz4 as a shared library? How do you intend it to operate when a project embeds both hse and lz4 and their own copy of lz4 wins? Why does it need to be inlined anyway, just for performance micro-optimizations?

That being said, I realize now I should obviously have reviewed the upstream meson.build, mea culpa.

I'm declaring a review of the upstream files to be a merge blocker, BTW.

https://github.com/hse-project/hse/blob/master/subprojects/packagefiles/lz4/lib/meson.build#L6

It is a compile-time define used by lz4 itself. We support compiling against system installed lz4, but performance will suffer. Our lz4 symbols are hidden so there shouldn't be any conflict there if I understand symbol visibility correctly. For instance, we already have this problem with lz4 and xxhash where lz4 includes a copy of xxhash. No symbol collision to speak of.

https://github.com/hse-project/hse/blob/master/lib/meson.build#L98

upstream meaning HSE or lz4?

@tristan957 '-DLZ4LIB_VISIBILITY=__attribute__((visibility("hidden")))', that should be dealt with gnu_symbol_visibility, and that seems to be an important fix that must go in wrapdb's lz4.

that should be dealt with gnu_symbol_visibility

That causes lz4 to go through the PLT:

gnu_symbol_visibility: 'hidden' & no LZ4 define

$ objdump -r -dx -Mintel --prefix-addresses ./build/lib/libhse-2.so | grep '<.*@plt> jmp' | egrep '(LZ4)'
000000000003b1d0 <LZ4_compressBound@plt> jmp    QWORD PTR [rip+0x109f12]        # 00000000001450e8 <LZ4_compressBound@@Base+0x5e228>
000000000003b260 <LZ4_initStream@plt> jmp    QWORD PTR [rip+0x109eca]        # 0000000000145130 <LZ4_initStream@@Base+0x5c3a0>
000000000003b510 <LZ4_compress_default@plt> jmp    QWORD PTR [rip+0x109d72]        # 0000000000145288 <LZ4_compress_default@@Base+0x5b2a8>
000000000003b730 <LZ4_decompress_safe_partial@plt> jmp    QWORD PTR [rip+0x109c62]        # 0000000000145398 <LZ4_decompress_safe_partial@@Base+0x55de8>
000000000003b920 <LZ4_createStream@plt> jmp    QWORD PTR [rip+0x109b6a]        # 0000000000145490 <LZ4_createStream@@Base+0x59f60>
000000000003ba10 <LZ4_decompress_safe@plt> jmp    QWORD PTR [rip+0x109af2]        # 0000000000145508 <LZ4_decompress_safe@@Base+0x564f8>
000000000003ba50 <LZ4_compress_fast_continue@plt> jmp    QWORD PTR [rip+0x109ad2]        # 0000000000145528 <LZ4_compress_fast_continue@@Base+0x59db8>
000000000003bbb0 <LZ4_resetStream@plt> jmp    QWORD PTR [rip+0x109a22]        # 00000000001455d8 <LZ4_resetStream@@Base+0x5a078>
000000000003bbe0 <LZ4_compress_fast@plt> jmp    QWORD PTR [rip+0x109a0a]        # 00000000001455f0 <LZ4_compress_fast@@Base+0x5b640>
000000000003bf40 <LZ4_resetStream_fast@plt> jmp    QWORD PTR [rip+0x10985a]        # 00000000001457a0 <LZ4_resetStream_fast@@Base+0x5a230>
000000000003bf60 <LZ4_versionString@plt> jmp    QWORD PTR [rip+0x10984a]        # 00000000001457b0 <LZ4_versionString@@Base+0x5e900>
000000000003bf90 <LZ4_decompress_fast@plt> jmp    QWORD PTR [rip+0x109832]        # 00000000001457c8 <LZ4_decompress_fast@@Base+0x55ca8>
000000000003bfd0 <LZ4_decompress_safe_withPrefix64k@plt> jmp    QWORD PTR [rip+0x109812]        # 00000000001457e8 <LZ4_decompress_safe_withPrefix64k@@Base+0x558a8>
000000000003c000 <LZ4_compress_fast_extState@plt> jmp    QWORD PTR [rip+0x1097fa]        # 0000000000145800 <LZ4_compress_fast_extState@@Base+0x5ca30>

no gnu_symbol_visibility and lz4 define

$ objdump -r -dx -Mintel --prefix-addresses ./build/lib/libhse-2.so | grep '<.*@plt> jmp' | egrep '(LZ4)'
000000000003ad20 <LZ4_decompress_safe_forceExtDict@plt> jmp    QWORD PTR [rip+0x10996a]        # 0000000000144690 <LZ4_decompress_safe_forceExtDict@@Base+0x55240>

From a quick look, you should do both. gnu_symbol_visibility: 'hidden' to make all symbols private by default, and -DLZ4LIB_VISIBILITY=__attribute__((visibility("default"))) to export public API. That's a bug in upstream meson port.

After doing both, I get 0 PLT calls whereas previously I had one. Not bad. Will update HSE accordingly.

@tristan957 lz4/lz4#1029.

@xclaesse why did that PR use cc.has_argument? :o

Also shouldn't there be an upstream option to inline the code rather than having downstreams fork the build?

@xclaesse why did that PR use cc.has_argument? :o

-fvisibility is a GNU thing, with msvc you need dllexport.

Also shouldn't there be an upstream option to inline the code rather than having downstreams fork the build?

I don't think it should be optional, it is just a way to flag which symbols are public API and which is internal.

FWIW lz4 seems to fail to link if you add the visibility define to a shared library. I added some protection for my downstream wrap in that case.

I know it's a GNU thing, that's why the kwarg you mentioned here begins with gnu_. I'm curious why you didn't use that kwarg in the PR...

@tristan957 right, because shared libraries exist to expose visible symbols but if you go ahead and hide them because you want them to be inline...

It was more a comment for @xclaesse upstream PR. I don't think he is protecting against it. Should've made the comment there.

I added a little bit of a hack to our lz4 wrap, so that it exports the same variable names as the upstream wrap, so I think it could work, but we will have to wait till a 2.0.1 release which should happen pretty soon.

hse-project/hse@115070f

https://github.com/hse-project/hse/blob/2b4bdbffb4219410e6e19194c155b1c1b9710682/meson.build#L75-L76

if git.found() and run_command(git, 'rev-parse').returncode() == 0
    build_version = run_command(

This will fail if git is installed, and the superproject is a git repo but the subproject is not.

In the wrapdb test, you end up with:

#define HSE_VERSION_STRING "6913386"

(because the commit you want to merge into the wrapdb is commit 6913386)

As a general rule of thumb it looks like all of your subproject fallbacks do not use [provide] sections, but instead manually look up variables. This seems wrong...

minor nits

You use if get_option('tools') to check for a bunch of dependencies. This should probably be a feature option instead, which also avoids the need for the else clause.

Use of find_program('scripts/....') does not need to use join_paths or the / operator, since you don't have to handle absolute path joining and it's all internally handled as posix paths anyway... it also doesn't need required: true since that is the default.

exe_install_rpath is broken unless bindir is a direct subdirectory of prefix.

java/hsejni/src/ each have a meson.build doing nothing but going one subdir() deeper. Just add a subdir('java/hsejni/src') and then keep the final meson.build.

https://github.com/hse-project/hse/blob/2b4bdbffb4219410e6e19194c155b1c1b9710682/lib/meson.build#L1-L6

build_config = ' \\\n\t' + ' \\\n\t'.join([
    '"-Dbuildtype=@0@ -Dforce_fallback_for=@1@ -Doptimization=@2@ "'.format(get_option('buildtype'), ','.join(get_option('force_fallback_for')), get_option('optimization')),
    '"-Ddebug=@0@ -Db_ndebug=@1@ "'.format(get_option('debug'), get_option('b_ndebug')),
    '"-Db_lto=@0@ -Db_pgo=@1@ -Dexperimental=@2@ "'.format(get_option('b_lto'), get_option('b_pgo'), get_option('experimental')),
    '"-Dc_args=\\"@0@\\" -Dc_link_args=\\"@1@\\""'.format(' '.join(get_option('c_args')), ' '.join(get_option('c_link_args')))
])

Seems like you really want to join this:

foreach opt: ['buildtype', 'force_fallback_for', 'optimization', 'debug', 'b_ndebug', 'b_lto', 'b_pgo', 'experimental']
    confvals += '"-D@0@=@1@"'.format(opt, get_option(opt))
endforeach

    # Make sure every file is executable
    run_command(
        sh,
        '-c',
        '[ -x "@0@" ]'.format(t),
        check: true
    )

For the record, since you pass every one of these as arguments to bash you probably do not care if it is executable.

foreach t, params : tools
    if get_option('tools')
        target = executable(

This if/else could be simplified if one of the deps there was a disabler. This ties back into my earlier comment about using feature options.

It seems that lz4 is the only one using a forked meson.build which deviates from upstream.

Cool. I submitted a PR to HSE, so hopefully I can get the approvals I need and it should be updated for 2.0.1. Got our build to respect default_library now as well.

PR will be put on hold until our 2.1.0 release later this year.

It would be great if at some point the lz4 upstream build system could be improved in time to be incorporated in that release. ;)

Once this PR is merged and released on wrapdb (lz4/lz4#1064), I am happy to drop my custom lz4 in favor of the wrapdb build. Then that should unblock this PR maybe, but won't be mergeable until HSE 3.0 which should be some time this year.

Update for myself. lz4 PR was merged, HdrHistogram_c PRs were merged (yay) and released. At this point HSE is waiting only on an lz4 release, which is taking a while...

Some of the upstream HSE build has changed since this PR was originally created. We are currently working on 3.0, so I will update the PR when that release comes out assuming that lz4 gets a new release.

This should be rebased against master. lz4 problem can be avoided for Ubuntu and macOS CI by supplying debian_ and brew_packages.

@neheb HSE needs access to a v1.9.4 of lz4 which doesn't exist since the Meson build that I updated for it hasn't be included in any release.

Oh I think I see what you are saying. You would also need to change -Dforce_fallback_for when configuring HSE. In any case, I think it is best to continue to wait for the next major release. The less users on this current release the better.