On 10/17/24 11:49 PM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
...
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.hYeah my first version of this used the uapi one but I thought doing that might conflict with snapshotting? Also it'd mean you'd absolutely have to have the $(TOOLS_INCLUDES) earlier in the include priority list and better maybe to special case in this instance.
Actually, I think the goal is to just stop using KHDR_INCLUDES (./usr/include) entirely!
More below...
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapiThe reasoning for this directory is further explained here:
tools/include/uapi/README(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known horror show. It's a difficult one to resolve as the UAPI pidfd.h header needs O_xxx defines but we also need to include this header in kernel code.
An #ifdef __KERNEL__ block might be a solution here but fixing that is out of scope for these changes.
Certainly out of scope! Your patch already avoids the biggest issue: it no longer requires "make headers", in order to build it. That's fine for now. Sorry to put you into the middle of a pre-existing kselftests debate.
And the #ifdef __KERNEL__ sounds like a potential solution, or at least a building block for one. I need to take a closer look at this particular header file mess, the fcntl.h situation is new to me.
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -WallI apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
Yeah I know, but this won't work due to the header conflict, I was doing this previously.
Also doing it this way means that uapi snapshot doesn't override the usr/ one if you have that, which I guess you want?
Actually, given that we want (or should want, so I claim) to build without first running "make headers", and given that "make headers" populates ./usr/include/, which in turn is what $(KHDR_INCLUDES) points to, this means that eventually we should end up with approximately:
CFLAGS += -g -isystem $(TOOLS_INCLUDES) -pthread -Wall
And I just checked, today's selftests/mm builds just fine, with a similar diff applied, so I'm not totally crazy:
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 02e1204971b0..b004a8edcba5 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -33,7 +33,7 @@ endif # LDLIBS. MAKEFLAGS += --no-builtin-rules
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) +CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(TOOLS_INCLUDES) LDLIBS = -lrt -lpthread -lm
TEST_GEN_FILES = cow
thanks,