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

chore: use upstream e-dant/watcher headers and build system #1119

Merged
merged 34 commits into from
Oct 31, 2024

Conversation

dunglas
Copy link
Owner

@dunglas dunglas commented Oct 26, 2024

Copy link
Collaborator

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -4,8 +4,7 @@

package watcher

// #cgo LDFLAGS: -lwatcher -lstdc++
// #cgo CFLAGS: -Wall -Werror
// #cgo LDFLAGS: -lwatcher-c -lstdc++ -Wl,-rpath /usr/local/lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this require the watcher to be installed to /usr/local/lib and/or does it affect static builds?

Copy link
Owner Author

@dunglas dunglas Oct 26, 2024

Choose a reason for hiding this comment

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

This can be override using CGO_LDFLAGS, but we'll likely be able to remove this soon: e-dant/watcher#59

@dunglas
Copy link
Owner Author

dunglas commented Oct 27, 2024

I don't get why we get this linking issue https://github.com/dunglas/frankenphp/actions/runs/11543390970/job/32127719767?pr=1119 (same in the Docker image). It works on my computer (^^) but I'm on Mac.

Maybe do you have an idea @e-dant?

@e-dant
Copy link
Contributor

e-dant commented Oct 27, 2024

I don't get why we get this linking issue https://github.com/dunglas/frankenphp/actions/runs/11543390970/job/32127719767?pr=1119 (same in the Docker image). It works on my computer (^^) but I'm on Mac.

Maybe do you have an idea @e-dant?

That looks correct to me. Running this locally in a container and strace'ing some stuff, the execve stuff also looks fine to me.

Attaching output from a run of

  • strace -e execve -v -s 9999 --follow-forks go run ./internal/testcli/main.go &> output.txt
  • with export CGO_CFLAGS='-I/usr/local/include -I/usr/include/php/20220829 -I/usr/include/php/20220829/main -I/usr/include/php/20220829/TSRM -I/usr/include/php/20220829/Zend -I/usr/include/php/20220829/ext -I/usr/include/php/20220829/ext/date/lib' ; export CGO_LDFLAGS='-L/usr/local/lib -lwatcher-c' in the environment
  • with watcher installed via git clone https://github.com/e-dant/watcher && (cd watcher && git checkout next) && cmake -S watcher/ -B watcher/out/tmp -DBUILD_BIN=ON -DBUILD_LIB=ON -DBUILD_HDR=ON && cmake --build watcher/out/tmp && cmake --install watcher/out/tmp

output.txt


The symbols must not be visible in the shared library. Everything else looks fine.

@e-dant
Copy link
Contributor

e-dant commented Oct 27, 2024

The symbols must not be visible in the shared library. Everything else looks fine.

strings /usr/local/lib/libwatcher-c.so.0.13.1 | grep wtr | wc -l
0

What?

@e-dant
Copy link
Contributor

e-dant commented Oct 28, 2024

One alternative is this:

$ wget https://github.com/e-dant/watcher/releases/download/0.13.1/$(arch)-unknown-linux-gnu.tar -qO- | tar -xvf - && nm -D $(arch)-unknown-linux-gnu/libwatcher-c.so.0.13.1 | rg ' T '
x86_64-unknown-linux-gnu/
x86_64-unknown-linux-gnu/tw
x86_64-unknown-linux-gnu/libwatcher-c.so
x86_64-unknown-linux-gnu/watcher-c.h
x86_64-unknown-linux-gnu/libwatcher-c.so.sha256sum
x86_64-unknown-linux-gnu/tw.sha256sum
x86_64-unknown-linux-gnu/watcher.sha256sum
x86_64-unknown-linux-gnu/watcher.hpp
x86_64-unknown-linux-gnu/watcher-c.h.sha256sum
x86_64-unknown-linux-gnu/libwatcher-c.so.0.13.1
x86_64-unknown-linux-gnu/watcher
x86_64-unknown-linux-gnu/libwatcher-c.so.0.13.1.sha256sum
x86_64-unknown-linux-gnu/watcher.hpp.sha256sum
000000000000799b T wtr_watcher_close
0000000000007e65 T wtr_watcher_open

Which does have these symbols...

Not sure how I broke the CMake build, but something is wrong there.

@e-dant
Copy link
Contributor

e-dant commented Oct 28, 2024

So, it turns out that our entire library was being optimized away with -fwhole-program...

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8f4a78e..98c6120 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -94,7 +94,6 @@ if(NOT WIN32 AND NOT IS_CC_MSVC)
     set(COMPILE_OPTIONS
       "${COMPILE_OPTIONS}"
       "-fexpensive-optimizations"
-      "-fwhole-program"
     )
   endif()
 endif()

This fixes it...

@e-dant
Copy link
Contributor

e-dant commented Oct 28, 2024

So, it turns out that our entire library was being optimized away with -fwhole-program...

Version 0.13.2 fixes this:

$ cmake -S watcher -B wout && cmake --build wout && cmake --install wout && go build frankenphp/internal/testcli/main.go

Should work

@dunglas
Copy link
Owner Author

dunglas commented Oct 28, 2024

Thank you very much @e-dant, this works!

@e-dant
Copy link
Contributor

e-dant commented Oct 31, 2024

❤️

@dunglas dunglas merged commit 69c43ee into main Oct 31, 2024
46 checks passed
@dunglas dunglas deleted the chore/watcher-c branch October 31, 2024 08:39
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.

4 participants