Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix inotify support to work with FreeBSD libinotify #39680

Closed
wants to merge 1 commit into from
Closed

Fix inotify support to work with FreeBSD libinotify #39680

wants to merge 1 commit into from

Conversation

rootwyrm
Copy link

FreeBSD provides a kqueue-inotify shim in the form of devel/libinotify. However, because it is not in an expected system location, it requires the full path. Otherwise it will fail as though inotify were not present. With this modification, corefx now accepts the libinotify shim, correctly identifies supported functions, and Just Works (for generous values of Works.)

FreeBSD provides a kqueue-inotify shim in the form of devel/libinotify. However, because it is not in an expected system location, it requires the full path. Otherwise it will fail as though inotify were not present. With this modification, corefx now accepts the libinotify shim, correctly identifies supported functions, and Just Works (for generous values of Works.)
@stephentoub
Copy link
Member

cc: @wfurt, @janvorli

@karelz karelz added this to the 5.0 milestone Aug 3, 2019
@wfurt
Copy link
Member

wfurt commented Aug 6, 2019

should we add /usr/local/include to include path?
It seems like we do not have full path for includes anywhere else.

cc: @joperator

@rootwyrm
Copy link
Author

rootwyrm commented Aug 7, 2019

@wfurt this is actually a rather... odd one. There's some intentional kludginess with libinotify to prevent random things from picking it up and assuming that the OS has real inotify versus the kqueue shim. Also bear in mind that corefx is not currently building standalone (but is from mono,) so it gets hit with a bit of a double-whammy of strangeness here due to autoconf.

./configure --prefix=/usr/local --disable-btls --include=/usr/include --include=/usr/local/include CC=clang CXX=c++ results in this:

configure:18589: checking sys/inotify.h usability
conftest.c:84:10: fatal error: 'sys/inotify.h' file not found
#include <sys/inotify.h>
| #include <sys/inotify.h>
configure:18589: checking sys/inotify.h presence
conftest.c:51:10: fatal error: 'sys/inotify.h' file not found
#include <sys/inotify.h>
| #include <sys/inotify.h>
configure:18589: checking for sys/inotify.h
configure:23106: checking for inotify_init in -linotify

ac_cv_func_inotify_add_watch=yes
ac_cv_func_inotify_init=yes
ac_cv_func_inotify_rm_watch=yes
ac_cv_header_sys_inotify_h=no
ac_cv_lib_inotify_inotify_init=yes
LIBS='-liconv -lm -lm   -linotify -pthread'

It's something about that specific location that seems to throw things off. If I copy /usr/local/include/sys/inotify.h to /usr/include/sys/inotify.h then I get a clean build without this patch. The problem with this however is that everything else in /usr/local/include picks up correctly - for example, System.Data.Sqlite3. It's something with that specific location, file, autoconf, or a combination of the three.

@joperator
Copy link

It looks like usr/local/include has already been added to the include paths in configure.cmake @wfurt:

elseif (CMAKE_SYSTEM_NAME STREQUAL FreeBSD)
    set(PAL_UNIX_NAME \"FREEBSD\")
    include_directories(SYSTEM /usr/local/include)

So, I think we should better investigate why this is not being applied to the failed project (System.Native I suppose @rootwyrm).

@wfurt
Copy link
Member

wfurt commented Aug 7, 2019

BTW what version do you use @rootwyrm? I was mostly working on 11.1

@rootwyrm
Copy link
Author

rootwyrm commented Aug 8, 2019

@wfurt 11.1 is EOL, so should be upgraded to 11.2-RELEASE at minimum (11.3 is the latest.) I've been testing with both 11.3 and 12.0 plus -CURRENT intermittently (expected to not work, of course.) Also, are you using the base compiler (clang) or a compiler from ports?

My suspicion is that something in there is just not at all happy with sys/inotify.h when it's in /usr/local/include but is perfectly fine with it in /usr/include - which testing seems to confirm. What has me utterly perplexed is that the compile test which uses <sys/inotify.h> DOES pass. Despite that, I haven't had any luck finding any change that actually makes pal_io.c compile other than this one.

@wfurt
Copy link
Member

wfurt commented Aug 8, 2019

When I was working on this 12.x was not out yet :) And yes, I was using clang from distro. I also was not aware of libinotify. I'll give it try over the weekend. There were some cases when we had to pull in more headers. (like check_struct_has_member)

With the source-build work, what version of FreeBSD do you have in mind @joperator? If 11.x is too old I can upgrade my VMs so we are more in sync.

@rootwyrm
Copy link
Author

rootwyrm commented Aug 8, 2019

Ha! I know that feeling. 11.2, 11.3, and 12.0 are the current supported releases though, so the hope is for it to work on all of those (but failing to work on unsupported versions is entirely accepted, and failing to work on 11.x would also be accepted but not preferred.) I'm happy to help any way I can there as I have an existing internal poudriere infrastructure so I can test multiple versions easily.

Pulling in the headers like you're talking about is definitely over my head - I'm not at all familiar with CMake. If you've got an idea there, I'm happy to test it.

@joperator
Copy link

@wfurt: I'm using a VM with FreeBSD 12.0.
@rootwyrm: I'm also not a CMake pro. For me it looks like it should work, but obviously it doesn't. Maybe someone from this project who wrote the CMake files can take a look at this problem.

@wfurt
Copy link
Member

wfurt commented Aug 9, 2019

I put up #40202.
The problem is that not only we need header but also linking needs to happen for detection to work.
It would be great if you both can do more testing.

@joperator
Copy link

I'm getting

/root/git/dotnet/corefx/eng/depProj.targets(101,5): error : Error no assets were resolved from NuGet packages. [/root/git/dotnet/corefx/eng/restore/tools/tools.depproj]
    0 Warning(s)
    1 Error(s)

when trying to run ./build.sh. I've already replaced every occurrence of netcoreapp2.1 with netcoreapp3.0 to make the bootstrap CLI work on FreeBSD. But now I'm struggling with this error that prevents me from testing your changes @wfurt. Have you encountered a similar error when trying to build dotnet/corefx @rootwyrm?

@@ -33,7 +33,10 @@
#elif HAVE_SENDFILE_4
#include <sys/sendfile.h>
#endif
#if HAVE_INOTIFY
#if defined(__FreeBSD__) & defined(HAVE_INOTIFY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be defined(HAVE_NOTIFY), but rather just HAVE_INOTIFY. The cmake configuration always define that symbol, only its value indicates whether the config time check succeeded or not.

@rootwyrm
Copy link
Author

@wfurt Sorry about the delay; I'm not even getting as far as you with build.sh using your #40202 . When I try to run the build, it's trying to fetch from https://dot.net/v1/dotnet-install.sh and timing out.

@stephentoub
Copy link
Member

@rootwyrm, @wfurt, what's the status here? Is this PR making forward progress or should it be closed for now?

@wfurt
Copy link
Member

wfurt commented Oct 21, 2019

I think this got replaced with #40202. With work at dotnet/source-build#1139, we should be able to run corefx tests again and verify functionality. But I did not get to it.

@rootwyrm
Copy link
Author

Yes, this was completed by #40202. I have tested and confirmed that is working as intended on FreeBSD - at least from within Mono builds - without any supplementary patching. Never did get any success with dotnet-install.sh due to missing FreeBSD binaries though.

@rootwyrm rootwyrm closed this Oct 21, 2019
@wfurt
Copy link
Member

wfurt commented Oct 21, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants