-
Notifications
You must be signed in to change notification settings - Fork 138
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
Can't build cleanly on Ubuntu 18.04 #258
Comments
For what it’s worth, I would like to see PR #257 included in the next release. |
Here's the unittest failure:
|
I'm not seeing any error with regards to libpq-fe.h, but the postgres test is being skipped for some reason I haven't yet been able to figure out.
Here's the Dockerfile I'm using to test: FROM ubuntu:18.04
ARG GEARMAN_REPO=https://github.com/gearman/gearmand
# Configure environment
ENV DEBIAN_FRONTEND=noninteractive \
TZ=America/New_York \
HOME=/root
# Install packages
RUN apt-get update \
&& apt-get -y upgrade \
&& apt-get -y install \
automake \
autoconf \
libtool \
make \
curl \
gcc \
g++ \
git \
gperf \
libboost-all-dev \
libevent-dev \
libhiredis-dev \
libssl-dev \
libpq-dev \
libtokyocabinet-dev \
tcsh \
python3-sphinx \
uuid-dev \
wget \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
RUN cd /tmp && git clone --depth 1 ${GEARMAN_REPO}.git
###RUN groupadd gearman \
### && useradd -r -d /var/lib/gearman -g gearman gearman
WORKDIR /tmp/gearmand
RUN ./bootstrap.sh -a
###RUN chown -R gearman:gearman /tmp/gearmand
###USER gearman
RUN ./configure --enable-ssl 2>&1 | tee ./configure.log
RUN make 2>&1 | tee ./build.log
RUN make test 2>&1 | tee ./test.log |
The unittest failure was also reported in issue #200 with gcc 7.3.0, but other unittests besides this one also failed there, I think? I don't understand why we don't see this in Travis CI. |
I don't really get the point of these unittests or why the test is different on macOS and FreeBSD. If I change the test, like so: --- /path/to/original/gearmand/libtest/unittest.cc 2019-11-27 17:02:45.553210000 -0500
+++ ./unittest.cc 2019-12-15 01:37:31.932691000 -0500
@@ -663,2 +663 @@
- ASSERT_EQ(Application::SUCCESS, true_app.run(args));
- ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.join());
+ ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.run(args)); It passes on Ubuntu 18.04. |
Apparently, the postgres test is skipped unless you set the environment variable
I think that result is fine, considering PostgreSQL is not actually running? @SpamapS: Are you sure Also, should I submit a PR to change |
By the way, a lot of the tests in We probably need something like
in Alternatively, could do something like this in #if (defined(__APPLE__) && __APPLE__) || (defined(__FreeBSD__) && __FreeBSD__)
#define TRUE_CMD "/usr/bin/true"
#else
#define TRUE_CMD "/bin/true"
#endif Thoughts? |
I'll confirm the patch solves the issue. Here an other version of the patch. diff --git a/libtest/unittest.cc b/libtest/unittest.cc
index 30f94d53..beed916f 100644
--- a/libtest/unittest.cc
+++ b/libtest/unittest.cc
@@ -655,14 +655,7 @@ static test_return_t application_doesnotexist_BINARY(void *)
true_app.will_fail();
const char *args[]= { "--fubar", 0 };
-#if defined(__APPLE__) && __APPLE__
- ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.run(args));
-#elif defined(__FreeBSD__) && __FreeBSD__
ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.run(args));
-#else
- ASSERT_EQ(Application::SUCCESS, true_app.run(args));
- ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.join());
-#endif |
|
Please elaborate. Why do you prefer that? How would that work? |
In my opinion global configuration's amendment for only testing reason should be avoided.
As you proposed with preprocessor directive |
Oh, OK, that makes sense. I thought you were proposing something else. 🙂 |
So, where are we at? I'll try the postgres envvar when I get a few minutes, but it seems like the unittest problem needs a PR? Can somebody submit? I may have some time in the holiday, but probably not. Would love to get a release out in January. |
I would, but it’s not obvious to me what the proper solution is. Various questions remain unanswered. Why does unittest do something different on Linux vs. BSD/Mac? Why did it stop working in Ubuntu 18.04? Will the new/BSD method work on other/older Linux distributions or does it need to be employed only on Ubuntu 18.04? If the latter, how do we do that? |
I changed the above Dockerfile to build on Ubuntu 16.04 (Xenial, gcc 5.4.0) and 14.04 (Trusty, gcc 4.8.4): ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.run(args)); works only on Ubuntu 18.04. It definitely fails on 16.04 and 14.04. On those distributions ASSERT_EQ(Application::SUCCESS, true_app.run(args));
ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.join()); passes, of course. How do we handle this? I think the most straightforward solution is to just remove this test. Any other solution seems complicated, and it's not really testing gearmand per se. |
Anyone have any feedback on this? |
How about this? Application::error_t return_value = true_app.run(args);
if (return_value == Application::INVALID_POSIX_SPAWN)
ASSERT_EQ(Application::INVALID_POSIX_SPAWN, return_value);
else
{
ASSERT_EQ(Application::SUCCESS, return_value);
ASSERT_EQ(Application::INVALID_POSIX_SPAWN, true_app.join());
} I've tested this on Ubuntu 14.04, 16.04, and 18.04, and it works on all of them. If nobody has a better idea, I'll submit a PR soon. |
LGTM. |
Issue #258: Fix t/unittest failure on Ubuntu 18.04 and location of true command on Linux
Nice work everybody. The libpq thing was actually caused or at least made worse by something broken on my build box. Clean Ubuntu 18.04 was able to build properly with the latest master. Assuming Travis is clean, I'll get a new release out soon! |
In trying to make a release from the commits since 1.1.18, I have run in to a number of build and test failures. Wondering if anyone else has them. Specifically,
I don't really have the time to debug deeply.. hoping somebody has some cycles to get through this and confirm that things work on Ubuntu 18.04.
The text was updated successfully, but these errors were encountered: