yajl: Fix CFLAGS in pkgconfig

[roolebo]

libvirt can't be built from source because yajl contains CFLAGS in
pkgconfig not matching layout of installed headers:

  ../src/util/virjson.c:36:11: fatal error: 'yajl/yajl_gen.h' file not
  found
  # include <yajl/yajl_gen.h>
            ^~~~~~~~~~~~~~~~~
  1 error generated.

Here's pkg-config output from meson logs:

  Called `/opt/homebrew/bin/pkg-config --cflags yajl` -> 0
  -I/opt/homebrew/Cellar/yajl/2.1.0/include/yajl

Here's actual layout of includedir:

  /opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_tree.h
  /opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_gen.h
  /opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_common.h
  /opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_version.h
  /opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_parse.h

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

---

[carlocab]

Thanks for your PR, @roolebo

I'm a bit confused about the build failure you're seeing since it didn't emerge in #74349. The yajl bottles have also been around for a while (the oldest bottle dates back to Mavericks!), and this hasn't caused anyone problems before.

That said, I do think the correct way to include headers from yajl is with an #include <yajl/generic_yajl_header.h> directive. Still, it'd be useful to know what's going on first.

[Gcenx]

@carlocab thats simple is due to the Intel homebrew install location being /usr/local.

By default /usr/local/include would be searched, this search patch becomes /usr/local/include/yajl/generic_yajl_header.h.

Now the problem is if your using a none default searched system location it becomes very different, I’ll use the M1 install location.

So now using /opt/homebrew, pkg-config will pass the location of /opt/homebrew/include/yajl, seems fine but this ends up becoming /opt/homebrew/yajl/yajl/generic_yajl_header.h causing the header to not be found.

[roolebo]

@carlocab The build failure happens if you checkout libvirt sources from git for development and try to build it with meson build && ninja -C build on M1.

I believe the upstream issue didn't hit Linux distributions because yajl headers reside in /usr/include/yajl and despite CFLAGS from pkg-config are -I/usr/include/yajl, yajl headers on Linux can be found because /usr/include is default include directory, so #include <yajl/generic_yajl_header.h> works.

I agree with @Gcenx wrt M1.

[carlocab]

Still doesn't quite explain it, as libvirt built fine on ARM in #74349.

[roolebo]

@carlocab I don't know all details of buildenv used by homebrew but it might make intermediate symlinks for dependencies and that will be sufficient to build libvirt package.

It's not sufficient for libvirt development though, because of the outlined issue with headers :)

[carlocab]

I guess I just have the following reservations:

1. This could break the way other users use yajl. yajl's pc file shouldn't be specifying the include path that way, but it does, and that's how users have used it for a while now.
2. This will discard all bottles for systems older than Mojave, in favour of a use-case that we don't quite really support. In fact, if you're developing libvirt, you can probably manage on your own even with yajl's problematic pc file. Not sure I can say the same for most users who just want to install yajl from a bottle.

[Gcenx]

If libvert lists pkg-config as a build requirement then the header search should be fixed in there end.

I’ve done this in wine for detecting SDL2.

Alternatively it’s not difficult to append the headers search path locally.

[Bo98]

Still doesn't quite explain it, as libvirt built fine on ARM in #74349.

The superenv adds HOMEBREW_PREFIX/include as a default include path.

[roolebo]

@carlocab I don't see how the change should break anything. Except all bottles older than Mojave will be discarded, that's definitely true :)

1. As @Gcenx said, the issue doesn't impact homebrew on Intel bacause /usr/local/include is in default include path. However it impacts all developers with M1 that rely on pkg-config to use yajl. It's not libvirt-specific. It just happened that I wanted to build libvirt today and I've hit the issue. And we, likely, don't want to fix all packages in all world because the bug is caused by wrong package config file in yajl.

2. Many homebrew users are indeed developers and use packages from homebrew as dependencies for their projects. IMO flawless homebrew experience is a reason of its continued success :)

[carlocab]

I don't see how the change should break anything.

Would it not break my builds if I use pkg-config to specify my CFLAGS, but include yajl's headers as #include <generic_yajl.h> rather than #include <yajl/generic_yajl.h>?

[Gcenx]

Would it not break my builds if I use pkg-config to specify my CFLAGS, but include yajl's headers as #include <generic_yajl.h> rather than #include <yajl/generic_yajl.h>?

Using #include <generic_yajl.h> would work with the current pkg-config output as it will be searching the correct directory for the header.

[Gcenx]

Here’s what I did in wine to resolve the same problem https://source.winehq.org/git/wine.git/patch/e4fbae832c868e9fcf5a91c58255fe3f4ea1cb30

[roolebo]

@carlocab Official documentation and all three users of yajl (libvirt, uwsgi, kafkacat) in homebrew use #include <yajl/generic_yajl.h>. I'm not sure it's a good development practice to embrace workarounds at wrong place and create bigger chaos for cross-platform development.

Consider you develop a new feature for uwsgi, you have some changes, without proper pkg-config you need to either:

• Always remember that you need to modify include path or an environment variable before building a package
• or, carry a patch that won't ever be upstream (because it's wrong) and on every PR the patch has to be taken out

Also, official build instructions of the package won't work, so new developers will struggle without any reason.
It complicates development without any benefit.

[carlocab]

The workaround is as simple as appending -I$(brew --prefix)/include to your CPPFLAGS. That doesn't seem so onerous, especially for someone who's already hacking on the source of libvirt, uwsgi or kafkacat. If you don't want to keep doing that, you can edit yajl.pc locally.

Suppose, on the other hand, I'm a user who doesn't even know what a pre-processor is: all I know is that I want to install some piece of software that depends on yajl (maybe I even have a formula for this in my own tap), and I need to compile it from source since it isn't packaged by Homebrew, and this software actually does #include <yajl_tree.h>, etc, as opposed to #include <yajl/yajl_tree.h>, because that's what works for yajl.pc.

This patch has now broken the build for me. When I go ask upstream what happened, it'll take some time for them to figure out that Homebrew actually jumped the gun on yajl and patched yajl.pc to accommodate some other downstream projects.

Perhaps I could patch the source of the software I wanted to install, but that might be challenging since I don't really understand #include directives.

Also, official build instructions of the package won't work

This sounds like a shortcoming of the build instructions when they're designed for non-default builds of upstream projects without saying so.

[roolebo]

The workaround is as simple as appending -I$(brew --prefix)/include to your CPPFLAGS. That doesn't seem so onerous, especially for someone who's already hacking on the source of libvirt, uwsgi or kafkacat. If you don't want to keep doing that, you can edit yajl.pc locally.

It's not onerous, it doesn't seem to be correct. And I'm not messing up with my environment wrong way. I obviously rebuild the package with my fix and I no longer have the issue on my laptop but I wanted to help with fixing wrong pkg-config for other people who might hit the issue (upstream PR is submitted as well, but I wouldn't hold my breath until it get merged, because no PRs are merged for libyajl since 2015). And I'm sorry for being stubborn but I deeply want homebrew to be the best package manager :)

Suppose, on the other hand, I'm a user who doesn't even know what a pre-processor is: all I know is that I want to install some piece of software that depends on yajl (maybe I even have a formula for this in my own tap), and I need to compile it from source since it isn't packaged by Homebrew, and this software actually does #include <yajl_tree.h>, etc, as opposed to #include <yajl/yajl_tree.h>, because that's what works for yajl.pc.

I'm not sure a user (or, perhaps, a developer) who doesn't know what a preprocessor is will even try to use libyajl. It's a necessary knowledge for any C programmer.

Perhaps I could patch the source of the software I wanted to install, but that might be challenging since I don't really understand #include directives.

It wouldn't be correct. pkg-config provides a contract, if you use CFLAGS/LDFLAGS provided by pkg-config you should be able to use an external dependency without any workarounds :) That's the whole point about pkg-config. It says what compile and link flags should be used for an external dependency. If you don't believe me please read: https://www.freedesktop.org/wiki/Software/pkg-config/, and this guide to pkg-config might be helpful: https://people.freedesktop.org/~dbn/pkg-config-guide.html

This sounds like a shortcoming of the build instructions when they're designed for non-default builds of upstream projects without saying so.

Package environment has to be reasonable and follow established programming conventions and practices. In this case incorrect pkg-config file breaks established practices so I doubt a fix to libvirt would ever be upstreamable.

[Gcenx]

The problem with a large majority of projects like this is they expect the headers to be installed into /usr/include or /usr/local/include hence the #include <yajl/yajl_tree.h>, this caused headaches when not installing a package into a none default location.

As the upstream project has not accepted any merger requests nor commits since 2015 the projects that expect the headers in a fixed location should be altered to check pkg-config and use its output, if pkg-config is not installed the sub-directory should be checked as usual. However as pkg-config is considered a standard development tool that doesn't make much sense, but some projects still like to support not having pkg-config installed.

Here are some examples;

prefix=/opt/local
exec_prefix=/opt/local
libdir=/opt/local/lib
includedir=/opt/local/include

Name: FreeType 2
URL: https://freetype.org
Description: A free, high-quality, and portable font engine.
Version: 23.4.17
Requires:
Requires.private: zlib, libpng, libbrotlidec
Libs: -L${libdir} -lfreetype
Libs.private: -lbz2
Cflags: -I${includedir}/freetype2

prefix=/opt/local
libdir=${prefix}/lib
includedir=${prefix}/include

bindir=${prefix}/bin
glib_genmarshal=${bindir}/glib-genmarshal
gobject_query=${bindir}/gobject-query
glib_mkenums=${bindir}/glib-mkenums

Name: GLib
Description: C Utility Library
Version: 2.68.0
Requires.private: libpcre >=  8.31
Libs: -L${libdir} -lglib-2.0 -lintl
Libs.private: -Wl,-framework,CoreFoundation -Wl,-framework,Carbon -Wl,-framework,Foundation -Wl,-framework,AppKit -liconv -lm
Cflags: -I${includedir}/glib-2.0 -I${libdir}/glib-2.0/include

# sdl pkg-config source file

prefix=/opt/local
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: sdl2
Description: Simple DirectMedia Layer is a cross-platform multimedia library designed to provide low level access to audio, keyboard, mouse, joystick, 3D hardware via OpenGL, and 2D video framebuffer.
Version: 2.0.14
Requires:
Conflicts:
Libs: -L${libdir}  -lSDL2 
Libs.private:  -lm  -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,ForceFeedback -lobjc -Wl,-framework,CoreVideo -Wl,-framework,Cocoa -Wl,-framework,Carbon -Wl,-framework,IOKit
Cflags: -I${includedir}/SDL2  -D_THREAD_SAFE

prefix=/opt/local
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include/libpng16

Name: libpng
Description: Loads and saves PNG files
Version: 1.6.37
Requires.private: zlib
Libs: -L${libdir} -lpng16
Libs.private: -lz 
Cflags: -I${includedir}

As can be seen in the examples ether includedir or cflags has a directory name after include I could give more examples but I think the above is decent selection of know projects to show this is common.

[heroin-moose]

@Gcenx, you are missing the point here. The libpng manual tells you to include png.h directly:

#include <png.h>

So the pkg-config file reflects this demand by setting CFLAGS accordingly. yajl, on the other hand, uses <yajl/yajl_*.h> pattern (even internally). Look here for example:

[snip]

#include <yajl/yajl_common.h>

[snip]

Even if you can hack up something to use API headers like this:

#include <yajl_tree.h>

int
main(void)
{
        return 0;
}

you will still get the error, because yajl_tree.h will do #include <yajl/yajl_common.h> anyway!

So setting CFLAGS like this (/usr/local/foo/bar/include/yajl) breaks yajl. This is not a matter of taste and preferences, this directory is required to be present in one of the include directories that C preprocessor is aware of.

[Gcenx]

@heroin-moose the listed projects previously gave directions to use use a sub directory.

Most projects as I’ve said expect to be installed into a standard location, pkg-config files are provided but bugs are not lightly to be found outside of being installed into a none standard location.

I believe modifying this to “fix” upstream issues due to ancient instructions that doesn’t even mention pkg-config is a mistake, the upstream projects that use that should instead correct how they’re using the headers location from pkg-config.

[heroin-moose]

@Gcenx, current pkg-config file is buggy. Using it alone without placing yajl/ in /usr/include (or any other include directory in C preprosessor search paths) will result in a compilation failure. Please, read again my comment, I've described why.

[Gcenx]

@heroin-moose if the projects are modified correctly it won't fail to compile I've literally done exactly this for wine see configure: Don't prepend folder name for SDL.h.

[heroin-moose]

@Gcenx, why are you keep referring to other projects? Please, read what I write. The API headers of yajl itself use #include <yajl/yajl_common.h>. You can't fix this by fixing other projects that use yajl, because yajl itself is using this convention.

[Gcenx]

@heroin-moose gottach, the easiest possible fix would be modifying the .pc file.

[heroin-moose]

@Gcenx, this is what @roolebo is proposing.

[Gcenx]

@heroin-moose yeah somehow I wasn’t getting the headers themselves where doing this ugh.

[roolebo]

You can't fix this by fixing other projects that use yajl, because yajl itself is using this convention.

This is correct. The yajl example from: https://lloyd.github.io/yajl/ works with my fix:

/* Build with: cc yajl-example.c $(pkg-config --libs --cflags yajl) */
#include <yajl/yajl_parse.h>
#include <yajl/yajl_gen.h>

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static int reformat_null(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_null(g);
}

static int reformat_boolean(void * ctx, int boolean)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_bool(g, boolean);
}

static int reformat_number(void * ctx, const char * s, size_t l)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_number(g, s, l);
}

static int reformat_string(void * ctx, const unsigned char * stringVal,
                           size_t stringLen)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_string(g, stringVal, stringLen);
}

static int reformat_map_key(void * ctx, const unsigned char * stringVal,
                            size_t stringLen)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_string(g, stringVal, stringLen);
}

static int reformat_start_map(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_map_open(g);
}


static int reformat_end_map(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_map_close(g);
}

static int reformat_start_array(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_array_open(g);
}

static int reformat_end_array(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_array_close(g);
}

static yajl_callbacks callbacks = {
    reformat_null,
    reformat_boolean,
    NULL,
    NULL,
    reformat_number,
    reformat_string,
    reformat_start_map,
    reformat_map_key,
    reformat_end_map,
    reformat_start_array,
    reformat_end_array
};

static void
usage(const char * progname)
{
    fprintf(stderr, "%s: reformat json from stdin\n"
            "usage:  json_reformat [options]\n"
            "    -m minimize json rather than beautify (default)\n"
            "    -u allow invalid UTF8 inside strings during parsing\n",
            progname);
    exit(1);

}

int 
main(int argc, char ** argv)
{
    yajl_handle hand;
    static unsigned char fileData[65536];
    /* generator config */
    yajl_gen g;
    yajl_status stat;
    size_t rd;
    int retval = 0;
    int a = 1;

    g = yajl_gen_alloc(NULL);
    yajl_gen_config(g, yajl_gen_beautify, 1);
    yajl_gen_config(g, yajl_gen_validate_utf8, 1);

    /* ok.  open file.  let's read and parse */
    hand = yajl_alloc(&callbacks, NULL, (void *) g);
    /* and let's allow comments by default */
    yajl_config(hand, yajl_allow_comments, 1);

    /* check arguments.*/
    while ((a < argc) && (argv[a][0] == '-') && (strlen(argv[a]) > 1)) {
        unsigned int i;
        for ( i=1; i < strlen(argv[a]); i++) {
            switch (argv[a][i]) {
                case 'm':
                    yajl_gen_config(g, yajl_gen_beautify, 0);
                    break;
                case 'u':
                    yajl_config(hand, yajl_dont_validate_strings, 1);
                    break;
                default:
                    fprintf(stderr, "unrecognized option: '%c'\n\n",
                            argv[a][i]);
                    usage(argv[0]);
            }
        }
        ++a;
    }
    if (a < argc) {
        usage(argv[0]);
    }


    for (;;) {
        rd = fread((void *) fileData, 1, sizeof(fileData) - 1, stdin);

        if (rd == 0) {
            if (!feof(stdin)) {
                fprintf(stderr, "error on file read.\n");
                retval = 1;
            }
            break;
        }
        fileData[rd] = 0;

        stat = yajl_parse(hand, fileData, rd);

        if (stat != yajl_status_ok) break;

        {
            const unsigned char * buf;
            size_t len;
            yajl_gen_get_buf(g, &buf, &len);
            fwrite(buf, 1, len, stdout);
            yajl_gen_clear(g);
        }
    }

    stat = yajl_complete_parse(hand);

    if (stat != yajl_status_ok) {
        unsigned char * str = yajl_get_error(hand, 1, fileData, rd);
        fprintf(stderr, "%s", (const char *) str);
        yajl_free_error(hand, str);
        retval = 1;
    }

    yajl_gen_free(g);
    yajl_free(hand);

    return retval;
}

This one without yajl prefix doesn't work with existing pc file:

/* Build with: cc yajl-example-2.c $(pkg-config --libs --cflags yajl) */
#include <yajl_parse.h>
#include <yajl_gen.h>

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static int reformat_null(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_null(g);
}

static int reformat_boolean(void * ctx, int boolean)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_bool(g, boolean);
}

static int reformat_number(void * ctx, const char * s, size_t l)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_number(g, s, l);
}

static int reformat_string(void * ctx, const unsigned char * stringVal,
                           size_t stringLen)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_string(g, stringVal, stringLen);
}

static int reformat_map_key(void * ctx, const unsigned char * stringVal,
                            size_t stringLen)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_string(g, stringVal, stringLen);
}

static int reformat_start_map(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_map_open(g);
}


static int reformat_end_map(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_map_close(g);
}

static int reformat_start_array(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_array_open(g);
}

static int reformat_end_array(void * ctx)
{
    yajl_gen g = (yajl_gen) ctx;
    return yajl_gen_status_ok == yajl_gen_array_close(g);
}

static yajl_callbacks callbacks = {
    reformat_null,
    reformat_boolean,
    NULL,
    NULL,
    reformat_number,
    reformat_string,
    reformat_start_map,
    reformat_map_key,
    reformat_end_map,
    reformat_start_array,
    reformat_end_array
};

static void
usage(const char * progname)
{
    fprintf(stderr, "%s: reformat json from stdin\n"
            "usage:  json_reformat [options]\n"
            "    -m minimize json rather than beautify (default)\n"
            "    -u allow invalid UTF8 inside strings during parsing\n",
            progname);
    exit(1);

}

int 
main(int argc, char ** argv)
{
    yajl_handle hand;
    static unsigned char fileData[65536];
    /* generator config */
    yajl_gen g;
    yajl_status stat;
    size_t rd;
    int retval = 0;
    int a = 1;

    g = yajl_gen_alloc(NULL);
    yajl_gen_config(g, yajl_gen_beautify, 1);
    yajl_gen_config(g, yajl_gen_validate_utf8, 1);

    /* ok.  open file.  let's read and parse */
    hand = yajl_alloc(&callbacks, NULL, (void *) g);
    /* and let's allow comments by default */
    yajl_config(hand, yajl_allow_comments, 1);

    /* check arguments.*/
    while ((a < argc) && (argv[a][0] == '-') && (strlen(argv[a]) > 1)) {
        unsigned int i;
        for ( i=1; i < strlen(argv[a]); i++) {
            switch (argv[a][i]) {
                case 'm':
                    yajl_gen_config(g, yajl_gen_beautify, 0);
                    break;
                case 'u':
                    yajl_config(hand, yajl_dont_validate_strings, 1);
                    break;
                default:
                    fprintf(stderr, "unrecognized option: '%c'\n\n",
                            argv[a][i]);
                    usage(argv[0]);
            }
        }
        ++a;
    }
    if (a < argc) {
        usage(argv[0]);
    }


    for (;;) {
        rd = fread((void *) fileData, 1, sizeof(fileData) - 1, stdin);

        if (rd == 0) {
            if (!feof(stdin)) {
                fprintf(stderr, "error on file read.\n");
                retval = 1;
            }
            break;
        }
        fileData[rd] = 0;

        stat = yajl_parse(hand, fileData, rd);

        if (stat != yajl_status_ok) break;

        {
            const unsigned char * buf;
            size_t len;
            yajl_gen_get_buf(g, &buf, &len);
            fwrite(buf, 1, len, stdout);
            yajl_gen_clear(g);
        }
    }

    stat = yajl_complete_parse(hand);

    if (stat != yajl_status_ok) {
        unsigned char * str = yajl_get_error(hand, 1, fileData, rd);
        fprintf(stderr, "%s", (const char *) str);
        yajl_free_error(hand, str);
        retval = 1;
    }

    yajl_gen_free(g);
    yajl_free(hand);

    return retval;
}

It produces the error mentioned by @heroin-moose:

$ cc yajl-example-2.c $(pkg-config --libs --cflags yajl)
In file included from yajl-example-2.c:1:
/opt/homebrew/Cellar/yajl/2.1.0/include/yajl/yajl_parse.h:22:10: fatal error: 'yajl/yajl_common.h' file not found
#include <yajl/yajl_common.h>
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

As a side note I'm not sure how it would be possible to fix libvirt, because it uses pkg-config indirectly through meson:
https://github.com/libvirt/libvirt/blob/master/meson.build#L1445

And meson expects proper flags from pkg-config.

[Gcenx]

@roolebo so does libvirt compile after this change is applied?

[roolebo]

@Gcenx, yes with my fix anyone on M1 can compile libvirt using provided instructions: https://libvirt.org/compiling.html

[carlocab]

I'm not sure a user (or, perhaps, a developer) who doesn't know what a preprocessor is will even try to use libyajl. It's a necessary knowledge for any C programmer.

Yes, that's why the hypothetical user in my example wasn't even a C programmer. They just wanted to install software that made use of yajl.

If we want to patch yajl here (and I'm still not convinced this is the right place for it -- the right place is upstream), we should at least make it backward-compatible:

diff --git a/src/yajl.pc.cmake b/src/yajl.pc.cmake
index 6eaca14..dd01a30 100644
--- a/src/yajl.pc.cmake
+++ b/src/yajl.pc.cmake
@@ -1,9 +1,10 @@
 prefix=${CMAKE_INSTALL_PREFIX}
 libdir=${dollar}{prefix}/lib${LIB_SUFFIX}
-includedir=${dollar}{prefix}/include/yajl
+includedir=${dollar}{prefix}/include
+extra_includedir=${dollar}{includedir}/yajl
 
 Name: Yet Another JSON Library
 Description: A Portable JSON parsing and serialization library in ANSI C
 Version: ${YAJL_MAJOR}.${YAJL_MINOR}.${YAJL_MICRO}
-Cflags: -I${dollar}{includedir}
+Cflags: -I${dollar}{includedir} -I${dollar}{extra_includedir}
 Libs: -L${dollar}{libdir} -lyajl

[roolebo]

@carlocab there's no point in backward compatibility for this particular case because you can't compile any code that depends on yajl with existing pkg-config file. Please take a look at the message: #74516 (comment)

[carlocab]

Yes, I read your comment. I'm also not convinced that no one using yajl has a local workaround that could possibly be broken by your proposed change to yajl.pc.

Is there a downside to using a backward-compatible change?

[carlocab]

The only way your example would work is by using a workaround that would currently be required.

Ok. So? Why is the expectation for a proper example is "one that works with absolutely no workarounds"?

We're already in agreement that yajl.pc is broken: my concern is that trying to fix it in this way will break it for someone who has workarounds for the broken yajl.pc. It seems disingenuous to respond by saying that that's not valid because it needs a workaround. Of course it needs a workaround: that's the whole premise of the situation.

I really don’t see any project handling the headers in the manner your expecting when a workaround would still be needed even get it to work.

Maybe, maybe not. Not sure you can say that with absolute certainty, though.

Then this formula will be broken for Apple Silicon and Linux as upstream hasn’t made a commit since 2015.

We have lots of broken formulae because upstream hasn't fixed problems with it. Carrying around fixes for them isn't really what we do.

[Gcenx]

Ok. So? Why is the expectation for a proper example is "one that works with absolutely no workarounds"?

No it’s more nothing will work without a workaround.

We're already in agreement that yajl.pc is broken: my concern is that trying to fix it in this way will break it for someone who has workarounds for the broken yajl.pc.

It seems your apposed to a workaround due to it breaking some other slim possible workaround.

Maybe, maybe not. Not sure you can say that with absolute certainty, though.

That would only be resolved by using your example pc modification, I’m guessing your more included to use that than any others.

We have lots of broken formulae because upstream hasn't fixed problems with it. Carrying around fixes for them isn't really what we do.

This was more of a side question, as I was under the impression broken formulas would be removed, also would be removed if a project hasn’t been maintained/updated in over a year?

[heroin-moose]

@carlocab, so you're proposing not to fix a real broken package because of the imaginary broken package?

[carlocab]

No. Please see #74516 (comment).

[roolebo]

@carlocab I've done an investigation and Fedora and FreeBSD have the fix like I proposed.

And there's another much earlier PR to yajl (almost 7 years old) - lloyd/yajl#139 and another one (9 years old) - lloyd/yajl#77
And with your proposal we can be in a situation when someone will be making non-portable code that couldn't be compiled on FreeBSD and Fedora, CentOS and RHEL. So I'm keeping the essence of the patch but updating it to the earlier version.

[carlocab]

I did check what other distros were doing with this yesterday. Why doesn't Debian do this?

[Gcenx]

This issue won’t affect most Distros as they install packages into /usr

[andreabolognani]

I need to compile it from source since it isn't packaged by Homebrew, and this software actually does #include <yajl_tree.h>, etc, as opposed to #include <yajl/yajl_tree.h>, because that's what works for yajl.pc.

While GitHub is not the only place where code is hosted, it's widely used enough that it can serve as a statistically significant sample.

Searching for include <yajl/ across all C code hosted on GitHub results in just shy of 20k hits, whereas searching for include <yajl_ yields no results.

Based on this data, I'd say it's fair to conclude that the chances a non-developer will see a regression caused by this change while attempting to install some random piece of software are vanishingly small.

As a side note I'm not sure how it would be possible to fix libvirt, because it uses pkg-config indirectly through meson:
https://github.com/libvirt/libvirt/blob/master/meson.build#L1445

And meson expects proper flags from pkg-config.

libvirt could likely work around the broken yajl.pc the same way it currently does for readline.pc, which for a very long time has been broken upstream and was affecting both FreeBSD and macOS in the same way we're now seeing for yajl.

I have provided upstream with a patch fixing readline.pc a couple of years ago but, even though it was accepted extremely quickly and with zero fuss, Readline's release cadence is quite slow and so it's taken a while for it to trickle downstream...

It looks like Readline 8.1 has finally been released last December, and both FreeBSD ports and Homebrew have quickly picked it up! That means the libvirt kludge is no longer necessary, and in fact I have just posted a patch dropping it :)

Incidentally, this means that Readline - a much more popular package than yajl - has been affected by a change in behavior much like the one that's been proposed here for about four months. Did you get reports of breakages introduced by it? I've skimmed the list of issues and I haven't been able to find any.

I did check what other distros were doing with this yesterday. Why doesn't Debian do this?

As others have pointed out, Linux will generally not be hit by this because headers are always installed under /usr/include, which is in the default header search path. That's also the reason why this kind of issue is introduced in the first place O:-)

That said, yajl.pc is no less broken in Debian than it is in Homebrew, so I'll file a bug against the package and provide the downstream maintainer with a patch fixing it.

[sjackman]

Searching for include <yajl/ across all C code hosted on GitHub results in just shy of 20k hits, whereas searching for include <yajl_ yields no results.

GitHub Search tokenizes entire words, so you can't search for a partial word. Searching for "include <yajl_tree.h>" language:C language:C++ language:Objective-C yields 105 hits, which is still much less than 20k.

https://github.com/search?q=%22include+%3Cyajl_tree.h%3E%22+language%3AC+language%3AC%2B%2B+language%3AObjective-C&type=Code

[Gcenx]

I’m looking through the results and currently none of those would be broken by this change as they ship there own modified of yajl.

[andreabolognani]

GitHub Search tokenizes entire words, so you can't search for a partial word. Searching for "include <yajl_tree.h>" language:C language:C++ language:Objective-C yields 105 hits, which is still much less than 20k.

Fair point. Note, however, that almost all of those hits are from projects that vendor their own copy of yajl, and thus would not be affected at all by this change.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[andreabolognani]

Please don't let this go stale. It's a good change, and the sooner it hits the repo the better :)

[carlocab]

I agree that it's a good change for upstream to make. We're not upstream, though, and I'm not sure I like the idea of carrying around another patch that will never get merged.

[heroin-moose]

Guys, almost two months passed. Either commit the change or close the ticket and tell people that it would not be possible to build libvirt on OS X until libvirt moves to some other JSON library.

[SMillerDev]

it would not be possible to build libvirt on OS X until libvirt moves to some other JSON library.

[andreabolognani]

FYI, libvirt's build system now contains a workaround specifically for this issue. See libvirt/libvirt@cd76a97.

I agree that it's a good change for upstream to make. We're not upstream, though, and I'm not sure I like the idea of carrying around another patch that will never get merged.

Carrying downstream patches is always a maintenance burden, so I fully understand your reluctance.

In the case of yajl, however, upstream development having stopped results in two consequences: the first one is that almost zero non-trivial maintenance has been necessary for years, and the second one is that, if you added a downstream-only patch to fix CFLAGS, you would not have to worry about rebasing it every single time a new upstream release comes out :)

Notice how the yajl patch is a single line change, whereas the libvirt kludge is almost twenty lines of code; additionally, each and every consumer of yajl will need to implement and carry a similar workaround, whereas if things were patched in Homebrew nobody else would have to ever worry about it.

Overall, I still think applying the patch to Homebrew would be the second-best solution - the actual best solution being, of course, a new upstream release that includes the fix, but we all know we can't really hope to ever get that :(

tell people that it would not be possible to build libvirt on OS X until libvirt moves to some other JSON library

We would love to use a different JSON library, and in fact a few years ago we even managed to switch to jansson for a brief time; however, that change never made into a release because we immediately found it caused a number of regressions and had to revert it. Since QEMU emits JSON that's not entirely standards-compliant, most JSON library are unfortunately just not usable for our purposes :(

[carlocab]

I'm willing to reconsider my position if another project (other than libvirt) asks for this, as long as no other Homebrew/core maintainer objects.

[Gcenx]

Here a few
i3, io, kafkacat, raptor2

Edited;
Should be io not lo

[carlocab]

Do you represent any of these projects, @Gcenx?

[Gcenx]

@carlocab no I’m not involved in those, those were just a couple of projects that would benefit from this proposed change.

[roolebo]

The workaround in libvirt no longer works. Is it possible to reconsider the PR?

[carlocab]

Fine with me, but I won't argue against anyone else objecting to it. Please open a new PR, though.

Locking this now to avoid filling up the inboxes of anyone else still following this. Those still interested can look at the new PR.