Help finding Brotli headers when doing VMR build under FreeBSD

[sec]

Small change to make VMR build complete under FreeBSD. It's the only patch we use right now, to build v9. fyi @arrowd @Thefrank

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

[arrowd]

My patch for the FreeBSD port was half-baked. The proper solution to this, IMO, is to call pkg_check_modules() inside the extra_libs.cmake and never do find_library() for brotli stuff, because pkgconfig takes care of it too.

[jkoritzinsky]

This won't work for RHEL-based distros as their brotli package doesn't provide the .pc files. Can you add a fallback for if pkg-config can't find it?

[Thefrank]

If this is just FreeBSD related would it be more efficient to run an extra check on FreeBSD only via CMake directive (e.g., CLR_CMAKE_TARGET_XXXXX)?

[am11]

Does this work:

Suggested change

	find_package(PkgConfig REQUIRED)
	pkg_check_modules(BROTLI REQUIRED libbrotlicommon)
	include_directories(${BROTLI_INCLUDE_DIRS})
	find_package(Brotli REQUIRED)

This is how we are locating OpenSSL, ZLIB etc. without pkgconfig.

[arrowd]

CMake doesn't seem to provide FindBrotli.cmake

[arrowd]

This won't work for RHEL-based distros as their brotli package doesn't provide the .pc files. Can you add a fallback for if pkg-config can't find it?

Then this change is fine as it is. The problem with FreeBSD is that Brotli is installed into a directory that is not searhed by the compiler by default. On Linux it is usually installed into /usr/include which is why it is working there.

[jkoritzinsky]

This change is not fine as it is, as the REQUIRED in pkg_check_modules will cause it to fail. I know this because I tried this exact thing a few months ago and the RHEL maintainers opened #109105

[Thefrank]

Are we back to?

if(CMAKE_SYSTEM_NAME STREQUAL FreeBSD)
  set(CMAKE_REQUIRED_INCLUDES /usr/local/include)
endif(CMAKE_SYSTEM_NAME STREQUAL FreeBSD)

The system compiler will not search /usr/local/include but the ports version should AFAIK

[arrowd]

No, hardcoding /usr/local is also a wrong thing to do. Let's just remove REQUIRED then and wrap include_directories into if (BROTLI_FOUND)?

[Thefrank]

No, hardcoding /usr/local is also a wrong thing to do.

Thanks for the heads up! FreeBSD does this in five different parts in runtime for various workarounds. OSX does this once too with a similar note about system compiler ignoring paths /usr/local paths. Wrong is wrong, so the hacky workarounds will need to go.

When I have time later this weekend, I will look into getting these redone to something that does not hardcode the path on FreeBSD. OSX will need to be looked at by someone else with OSX experience. Maybe the behavior has changed since the workaround was put in?

[arrowd]

Thanks for the heads up! FreeBSD does this in five different parts in runtime for various workarounds. OSX does this once too with a similar note about system compiler ignoring paths /usr/local paths. Wrong is wrong, so the hacky workarounds will need to go.

These changes are currently masking the real problems and removing include_directories(/usr/local/include) calls will reveal them.

To explain better why hardcoding /usr/local is wrong. When building some piece of software it is usually possible to define the location where it is going to be installed. In ports Makefiles this location is defined by the PREFIX variable which defaults to ${LOCALBASE}. LOCALBASE is another variable that defines the location where all 3rd-party software is installed on a given FreeBSD system and defaults to /usr/local.
Now, all this hardcoded stuff will work until the end-user user decide to change LOCALBASE to something else than /usr/local. For example, it is possible to setup FreeBSD in such way that all 3rd-party packages would be installed into /opt.
For the upstream projects all this means that they should not rely on hardcoded paths, but rather perform a search step like CMake's Find*.cmake modules or calling pkg-config.

When I have time later this weekend, I will look into getting these redone to something that does not hardcode the path on FreeBSD.

This is appreciated. Feel free to get in touch with me if you need help with that.

[am11]

set(CMAKE_REQUIRED_INCLUDES /usr/local/include)

It's more like include_directories(SYSTEM ${CROSS_ROOTFS}/usr/local/include) to continue supporting cross-os build and matching what we are already using elsewhere.

To explain better why hardcoding /usr/local is wrong. When building some piece of software it is usually possible to define the location where it is going to be installed. In ports Makefiles this location is defined by the PREFIX variable which defaults to ${LOCALBASE}.

The best practices sound good. Keeping things consistent is even better because once we figure out a better approach, we can apply it to all places at once. Lets unblock VMR using the existing approach, then cleanup all instances repo-wide, i.e. git grep -B2 /usr/local/include -- :/ in a separate PR (with title indicating that it's a general overhaul).

[sec]

Made proposed change, hope I get it correctly.