Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve libinotify detection on BSDs #1101

Merged
merged 1 commit into from
Dec 22, 2024
Merged

Conversation

tleedjarv
Copy link
Contributor

No description provided.

@gdt
Copy link
Collaborator

gdt commented Dec 20, 2024

I tried to read the code and construct the test program build line:

ocamlc -ccopt "-I/usr/pkg/include" -cclib "-L/usr/pkg/lib -Wl,-rpath/usr/pkg/lib -linoti" inotifytest__.c && echo OK

and then I munged the lib to misspell (pasted above) and it still passed. It does fail without the -I part.

I think what's going on is that this is compiling the .c, but there is no link step so the unresolved symbol is not objected to.
In autoconf speak, I think we need AC_TRY_LINK, and not just AC_TRY_COMPILE.

src/make_tools.ml Outdated Show resolved Hide resolved
@tleedjarv
Copy link
Contributor Author

I think what's going on is that this is compiling the .c, but there is no link step so the unresolved symbol is not objected to. In autoconf speak, I think we need AC_TRY_LINK, and not just AC_TRY_COMPILE.

You're right... but now that I think about it, there's no need for this test at all. I wrote it before relying on pkg-conf. If we require pkg-conf then we don't need to do this test compile/link at all and can just use the output of pkg-conf directly, no?

@gdt
Copy link
Collaborator

gdt commented Dec 20, 2024

There's pkg-config succeeding, which means there's a .pc file, and then there's actually being able to link. I will see what the norms are in the autoconf world, but my gut reaction is to try to link.

(Separately, autoconf also prints out "checking for foo...yes" sorts of things and it would help debugging to have a yes/no about libinotify, sort of Checking for libinotify...[yes/no], followed by [Enabling fsmonitor/inotify.]/[Disabling fsmonitor.])

@gdt
Copy link
Collaborator

gdt commented Dec 20, 2024

I looked at two random programs that use pkg-config via autoconf, and they both just believe the output. Perhaps it is wise to use --exists first, and if true, just set flags based on --cflags/--libs.

@tleedjarv
Copy link
Contributor Author

I looked at two random programs that use pkg-config via autoconf, and they both just believe the output. Perhaps it is wise to use --exists first, and if true, just set flags based on --cflags/--libs.

Simplifying sounds like a good idea. Let's do just that. If this backfires in the future, we can always revisit this.

@gdt
Copy link
Collaborator

gdt commented Dec 22, 2024

Thanks for the multiple iterations. I have tested via pkgsrc-wip (before the latest if clarification but that's good enough) and it works both ways, with libinotify apparently present and apparently not present.

@gdt gdt merged commit 3b8d3d4 into bcpierce00:master Dec 22, 2024
31 checks passed
@tleedjarv tleedjarv deleted the fix-linotify branch December 22, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants