Paths

Table of Contentst

inotify.h into a subdirectory
AbandonedPublic
Actions

Authored by jbeich on Jan 27 2019, 10:38 AM.

Details

Reviewers

Summary

inotify is not supported by FreeBSD kernel, so prevent consumers from unintentionally picking up the shim library when it may not be desired.

Test Plan

Diff Detail

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 22192
Build 21404: arc lint + arc unit

Event Timeline

jbeich created this revision.Jan 27 2019, 10:38 AM

Harbormaster completed remote builds in B22190: Diff 53269.Jan 27 2019, 10:38 AM

Adjust commit message
Update build logs
Add a reviewer

Harbormaster completed remote builds in B22192: Diff 53271.Jan 27 2019, 11:47 AM

I have an issue with the pkg-config lines. Running binaries in makefiles is usually discouraged in our policies.

I see it a little brittle too. There is no way to warrant pkg-config has been installed when the variable is evaluated. In general making port Makefiles depend on other ports being installed looks problematic.

Apart from this I see the pkg-config lines duplicated a lot around.

I'd like to suggest an alternative approach: you could create a libinotify USES value which could add the dependency when added to USES and could add the required flags to LDFLAGS, CPPFLAGS and CFLAGS.

All the USES mk files set such flags based on LOCALBASE, so instead of running pkg-config you could simply do(example, which could be incorrect, just to show the idea):

CFLAGS="-I ${LOCALBASE}/include/libinotify"
LDFLAGS="-L ${LOCALBASE}/lib -linotify"

AFAIK this is correct, since the port installs them there and we actually don't support mixing ports with different LOCALBASE, and actually I'd expect ports installed in different LOCALBASE not to be able to find each other.

Could you try this kind of approach? It would be much cleaner, building such a USES makefile should be quite easy, the iconv one performs a similar task.

In D18990#405667, @madpilot wrote:

Running binaries in makefiles is usually discouraged in our policies.

CPPFLAGS+=`pkg-config --cflags libinotify 2>/dev/null` delays running pkg-config until the underlying configure script or submake tries to evaluate the expression as part of a shell command. This is a temporary hack until an upstreamable solution can be created. In some cases, upstream build system is too primitive (e.g., no configure stage) to do detect libinotify flags properly.

There is no way to warrant pkg-config has been installed when the variable is evaluated.

Before configure stage *FLAGS value is undefined i.e., incomplete and some ${...} variables maybe unset.

I'd like to suggest an alternative approach: you could create a libinotify USES value which could add the dependency when added to USES and could add the required flags to LDFLAGS, CPPFLAGS and CFLAGS.

Disadvantages:

yet another place where libinotify installation paths are hardcoded
*FLAGS from ports are global, so may lead to overlinking in shared libraries
whether to add USES=inotify to ports with proper upstream support for libinotify
ports may not respect *FLAGS, so patching would still be required

Could you try this kind of approach? It would be much cleaner, building such a USES makefile should be quite easy, the iconv one performs a similar task.

Cementing a workaround wouldn't help out-of-tree builds. I'm trying to keep upstreaming in mind rather than benefit only FreeBSD Ports users. For one, kf5-extra-cmake-modules, owncloud/nextcloud, sssd, obs-qtwebkit, kodi (dropped?) already have some libinotify support upstream.

USES=iconv is bad example. Did you actually look inside?

can be libc, libiconv or -DLIBICONV_PLUG shim
no pkg-config support, so every build system has to reinvent

Disclaimer: I'm not going to keep this patch at ransom on my whim for a better approach. My main concern is that using pkg-config in port Makefile is a bad practice.

I can live with pkg-config in the Makefiles of the ports I maintain though. Only I'd like to understand some points before accepting this.
I'd like to address this one point before everything else:

this is a temporary hack while looking for upstreamable solution

Maybe the discussion will be easier if you explain you strategy. What you plan top patch, and then upstream. Which ports? How many?

Experience teaches that upstreaming changes can require a lot of time and sometimes is impossible. Even when projects do listen and accept patches in a short time, a lot more time can pass before a new release, including the patches, is available.

My concern is that such a "temporary hack" will end staying in the tree for a long time.

Another point I can't understand is how this patch to the ports tree could help people building software not using the ports tree(for definition those will not even see this patch), and how it helps you creating patches and upstreaming them. This is no obstacle to my approval, but I'd like to understand it.

In D18990#405697, @jbeich wrote:

In D18990#405667, @madpilot wrote:

Running binaries in makefiles is usually discouraged in our policies.

CPPFLAGS+=`pkg-config --cflags libinotify 2>/dev/null` delays running pkg-config until the underlying configure script or submake tries to evaluate the expression as part of a shell command. This is a temporary hack until an upstreamable solution can be created. In some cases, upstream build system is too primitive (e.g., no configure stage) to do detect libinotify flags properly.

As stated above it's the "temporary hack" part that worries me.

There is no way to warrant pkg-config has been installed when the variable is evaluated.

Before configure stage *FLAGS value is undefined i.e., incomplete and some ${...} variables maybe unset.

I agree that for most cases it's going to work, but there is absolutely no warranty. Anyway I concede there are a bunch of ports doing this, so I'll let this point slip.

I'd like to suggest an alternative approach: you could create a libinotify USES value which could add the dependency when added to USES and could add the required flags to LDFLAGS, CPPFLAGS and CFLAGS.

Disadvantages:

yet another place where libinotify installation paths are hardcoded

This is not an issue. The paths are already hardcoded in the libinotify port plist. For all that matters to the ports tree that is the only one path that counts.

*FLAGS from ports are global, so may lead to overlinking in shared libraries

What do you mean with "global"? Only ports including the libinotify USES will have it's values in the *FLAGS. As you well know the libinotify USES inclusion can be controlled by an option.

In your patch you're anyway populating *FLAGS for the most part so I can't see the difference.

whether to add USES=inotify to ports with proper upstream support for libinotify

There is no dilemma here. Any port using it should include USES=libinotify (depending on an option if one wants that), so it gets the required depends and the required values in it's *FLAGS automatically. If the software has correct detection it should be unaffected.

ports may not respect *FLAGS, so patching would still be required

Software not respecting *FLAGS needs patching anyway in the ports tree. It's mandatory to make them respect those so I again don't see any difference with normal practice.

Could you try this kind of approach? It would be much cleaner, building such a USES makefile should be quite easy, the iconv one performs a similar task.

Cementing a workaround wouldn't help out-of-tree builds. I'm trying to keep upstreaming in mind rather than benefit only FreeBSD Ports users. For one, kf5-extra-cmake-modules, owncloud/nextcloud, sssd, obs-qtwebkit, kodi (dropped?) already have some libinotify support upstream.

A new USES is not cementing anything, in fact makes it quite easier to clean up any hacks afterwards. The hacks will be in one place only, instead of being spread in various ports.

Not sure also for what you mean by support here. I understand it that you mean some software has proper detection for libinotify presence or absence, while others do not, and assume they are running on linux with native inotify support.

The ports tree was born to address such issues. A USES would be better. Even if you succeed in getting patches upstreamed in all projects you have found in the tree with problems, this would not help with future ported software. A specific USES, instead, would be an instrument usable in the future too.

USES=iconv is bad example. Did you actually look inside?

I know that USES very well, I contributed to it. Maybe I did choose a bad example. I wanted to point only to some parts of it actually.

can be libc, libiconv or -DLIBICONV_PLUG shim

What a libinotify USES needs to do is add dependencies and populate *FLAGS appropriately, The *FLAGS population part can be removed once you have successfully upstreamed your patches.

Maybe I could have used a better example, but we can drop the example and just talk about what the hypothetical "USES" should do.

no pkg-config support, so every build system has to reinvent

What do you mean by "pkg-config' support? Leaving behing the iconv thing, what pkg-config support should it have? If you mean that the USES should derive CFLAGS and LDFLAGS content using pkg-config, I strongly disagree, since the ports tree will invariably install libnotify includes and libraries in ${PREFIX} subdirectories dictated by the plist, and other ports are going to expect to find them in the same subdirectories of ${LOCALBASE}.

jbeich abandoned this revision.Mar 27 2023, 7:27 PM