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

folly fizz wangle watchman 2021.08.02.00 #82474

Closed

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Aug 3, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Another try after #81599 with some Linux fixes.

Highly doubt macOS build of watchman has been fixed since upstream is only able to build Linux:
https://github.com/facebook/watchman/actions/runs/1089437312

@cho-m cho-m added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Aug 3, 2021
@cho-m
Copy link
Member Author

cho-m commented Aug 3, 2021

Same failure on macOS for watchman facebook/watchman#930

In file included from /tmp/watchman-20210802-98370-ui8q6d/watchman-2021.08.02.00/watchman/PendingCollection.h:10:
/tmp/watchman-20210802-98370-ui8q6d/watchman-2021.08.02.00/watchman/watchman_string.h:114:16: error: no member named 'data' in 'watchman_pending_fs'
      : s_(str.data()), e_(str.data() + str.size()) {}
           ~~~ ^

Linux failure for watchman with GCC-11:

/home/linuxbrew/.linuxbrew/include/gtest/gtest-printers.h:291:36: error: no matching function for call to 'testing::internal::internal_stream_operator_without_lexical_name_lookup::StreamPrinter::PrintValue(const watchman::lrucache::Node<std::__cxx11::basic_string<char>, bool>&, std::nullptr_t)'
  291 |     T, decltype(Printer::PrintValue(std::declval<const T&>(), nullptr)),
      |                 ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/linuxbrew/.linuxbrew/include/gtest/gtest-printers.h:212:15: note: candidate: 'template<class T, class, class> static void testing::internal::internal_stream_operator_without_lexical_name_lookup::StreamPrinter::PrintValue(const T&, std::ostream*)'
  212 |   static void PrintValue(const T& value, ::std::ostream* os) {
      |               ^~~~~~~~~~
/home/linuxbrew/.linuxbrew/include/gtest/gtest-printers.h:212:15: note:   template argument deduction/substitution failed:
/home/linuxbrew/.linuxbrew/include/gtest/gtest-printers.h:211:33: error: no match for 'operator<<' (operand types are 'std::basic_ostream<char>' and 'const watchman::lrucache::Node<std::__cxx11::basic_string<char>, bool>')
  210 |             typename = decltype(std::declval<std::ostream&>()
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  211 |                                 << std::declval<const T&>())>
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~

@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch 2 times, most recently from c44786c to 518c63f Compare August 3, 2021 04:33
@danielnachun
Copy link
Member

You will need to add fails_with gcc: "11" to prevent this from trying to use GCC-11, which for some reason is still being installed.

@cho-m
Copy link
Member Author

cho-m commented Aug 3, 2021

You will need to add fails_with gcc: "11" to prevent this from trying to use GCC-11, which for some reason is still being installed.

I don't know for sure if GCC-9 will help on Linux. Just trying things out for now. I'll try disabling some newer versions of GCC in case it helps.

GCC-11 is able to compile folly, fizz, and wangle. I wanted to try an older GCC on watchman, but folly gets linked to GCC-11's glibc.

Still trying to figure out how upstream is able to build watchman on Linux successfully.
It is also possible the GoogleTest version we use has issues.

The Facebook projects seem to have their own build system, which will take some time to dissect and see what versions of dependencies are actually used in CI.

@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch from 518c63f to f4b9aa8 Compare August 4, 2021 00:40
@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch 2 times, most recently from 29a592f to 68df02d Compare August 9, 2021 11:26
@cho-m
Copy link
Member Author

cho-m commented Aug 9, 2021

Got watchman to build on ARM 🎉 (hopefully means other macOS will be good too).

But broke test for wangle while trying to fix Linux test issue.

Still need to find what boost linker flags are needed to get Linux to compile the wangle test program.

Error: test failed
/home/linuxbrew/.linuxbrew/bin/ld: /home/linuxbrew/.linuxbrew/opt/folly/lib/libfolly.so: undefined reference to symbol 'jump_fcontext'
/home/linuxbrew/.linuxbrew/bin/ld: /home/linuxbrew/.linuxbrew/lib/libboost_context-mt.so.1.76.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

@alebcay
Copy link
Member

alebcay commented Aug 9, 2021

@cho-m for the jump_fcontext missing symbol, it looks like this might help - needs -dl added (not tested myself).

@cho-m
Copy link
Member Author

cho-m commented Aug 9, 2021

@cho-m for the jump_fcontext missing symbol, it looks like this might help - needs -dl added (not tested myself).

I did see that and only tried the first flag here, which failed with cannot find -lboost_context.

It could be libdl helps as it is dynamic linkage library. Not very familiar with it personally.

I'll probably restart test with those flags inside an on_linux block to see.

@cho-m
Copy link
Member Author

cho-m commented Aug 9, 2021

We might also be hitting NixOS/nix#2306 where our libraries are -mt (is this multithread?)

# ls .linuxbrew/opt/boost/lib/libboost_context*
.linuxbrew/opt/boost/lib/libboost_context-mt.a   .linuxbrew/opt/boost/lib/libboost_context-mt.so.1.76.0
.linuxbrew/opt/boost/lib/libboost_context-mt.so

Might try using -lboost_context-mt instead

@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch from 68df02d to f1fb8a3 Compare August 9, 2021 12:12
@cho-m cho-m mentioned this pull request Aug 9, 2021
system "cmake", "-S", ".", "-B", "build",
"-DBUILD_SHARED_LIBS=ON",
"-DCMAKE_PREFIX_PATH=#{buildpath}/googletest",
Copy link
Member

Choose a reason for hiding this comment

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

Let's try ENV.append_path "CMAKE_PREFIX_PATH"; I don't think we want to override the env variable set by superenv.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try updating after Linux CI results are available to see if any changes are needed there.

Copy link
Member Author

@cho-m cho-m Aug 9, 2021

Choose a reason for hiding this comment

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

Actually, I don't think it overrides the value.

For example, in https://cmake.org/cmake/help/latest/command/find_package.html, this should behave similar to ENV.prepend_path since it is step 2 (Search paths specified in cmake-specific cache variables) while Homebrew superenv uses step 3 (Search paths specified in cmake-specific environment variables).

Since we want local GoogleTest to take precedence, this should behave as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

For more specific option, I think setting

ENV["GTest_DIR"] = ENV["GMock_DIR"] = buildpath/"googletest"

worked for me.

Giving this a try on CI instead of general search path.

@cho-m
Copy link
Member Author

cho-m commented Aug 9, 2021

Linux passes on everything but wangle. Got past the build error on EchoClient, but now timing out while building EchoServer. EDIT: Or may be while running the test? The lack of print output on some commands make it hard to tell where it is actually failing.

==> /home/linuxbrew/.linuxbrew/bin/g++-11 /home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/share/wangle/EchoClient.cpp -std=c++14 -I/home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/include -I/home/linuxbrew/.linuxbrew/opt/openssl@1.1/include -L/home/linuxbrew/.linuxbrew/opt/gflags/lib -L/home/linuxbrew/.linuxbrew/opt/glog/lib -L/home/linuxbrew/.linuxbrew/opt/folly/lib -L/home/linuxbrew/.linuxbrew/opt/fizz/lib -L/home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/lib -lgflags -lglog -lfolly -lfizz -lwangle -L/home/linuxbrew/.linuxbrew/opt/boost/lib -lboost_context-mt -ldl -lpthread -o EchoClient
==> /home/linuxbrew/.linuxbrew/bin/g++-11 /home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/share/wangle/EchoServer.cpp -std=c++14 -I/home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/include -I/home/linuxbrew/.linuxbrew/opt/openssl@1.1/include -L/home/linuxbrew/.linuxbrew/opt/gflags/lib -L/home/linuxbrew/.linuxbrew/opt/glog/lib -L/home/linuxbrew/.linuxbrew/opt/folly/lib -L/home/linuxbrew/.linuxbrew/opt/fizz/lib -L/home/linuxbrew/.linuxbrew/Cellar/wangle/2021.08.02.00/lib -lgflags -lglog -lfolly -lfizz -lwangle -L/home/linuxbrew/.linuxbrew/opt/boost/lib -lboost_context-mt -ldl -lpthread -o EchoServer
Killing child processes...
*** Aborted at 1628513237 (Unix time, try 'date -d @1628513237') ***
*** Signal 15 (SIGTERM) (0x3e800005e4d) received by PID 24134 (pthread TID 0x7f0fcb2884c0) (linux TID 24134) (maybe from PID 24141, UID 1000) (code: 0), stack trace: ***
(error retrieving stack trace)
Error: wangle: failed
An exception occurred within a child process:
Error: test failed
  Errno::EIO: Input/output error @ io_fread - /dev/pts/0
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/wangle.rb:86:in `read'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/wangle.rb:86:in `block in <class:Wangle>'

@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch 2 times, most recently from f47f063 to eeb0825 Compare August 9, 2021 18:35
@cho-m cho-m added the in progress Stale bot should stay away label Aug 9, 2021
@cho-m
Copy link
Member Author

cho-m commented Aug 9, 2021

Added a bunch of ohai/print statements to wangle to find out where it is timing out.

@cho-m cho-m force-pushed the bump-folly-fizz-wangle-2021.08.02.00 branch from eeb0825 to c90cea0 Compare August 9, 2021 23:27
@cho-m cho-m added ready to merge PR can be merged once CI is green and removed in progress Stale bot should stay away labels Aug 10, 2021
@cho-m
Copy link
Member Author

cho-m commented Aug 10, 2021

Successfully built on both macOS and Linux. Should be good to review and merge now.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Nice work!

Comment on lines +80 to +92
ohai "Starting EchoServer on port #{port}"
fork { exec testpath/"EchoServer", "-port", port.to_s }
sleep 2
sleep 3

require "pty"
r, w, pid = PTY.spawn(testpath/"EchoClient", "-port", port.to_s)
w.write "Hello from Homebrew!\nAnother test line.\n"
sleep 1
Process.kill("TERM", pid)
output = r.read
output = ""
PTY.spawn(testpath/"EchoClient", "-port", port.to_s) do |r, w, pid|
ohai "Sending data via EchoClient"
w.write "Hello from Homebrew!\nAnother test line.\n"
sleep 3
Process.kill "TERM", pid
begin
ohai "Reading received data"
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the ohai calls now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe do this in a CI-syntax-only PR to avoid rerunning. They do help though when running CI as it is very difficult to debug where the CI fails since we don't print out anything after compiling the test program.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@cho-m cho-m deleted the bump-folly-fizz-wangle-2021.08.02.00 branch August 10, 2021 06:34
@cho-m cho-m mentioned this pull request Aug 10, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants