Skip to content

ci: build and test tntcxx on macos 11 and 12 #55

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

Merged
merged 5 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions .github/workflows/ubuntu.yml → .github/workflows/testing.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: ubuntu
name: testing

on:
workflow_dispatch:
Expand All @@ -15,28 +15,43 @@ jobs:
runs-on:
- ubuntu-20.04
- ubuntu-22.04
tarantool:
- '2.11'
- macos-11
- macos-12
mode:
- Debug
- Release
test-ssl:
- Disabled

runs-on: ${{ matrix.runs-on }}

name: ${{ matrix.runs-on }} (${{ matrix.mode }}, ${{ matrix.test-ssl }} ssl test)

steps:
- name: Clone the connector
uses: actions/checkout@v3

- name: Setup tarantool ${{ matrix.tarantool }}
- name: Setup tarantool 2.11
if: startsWith(matrix.runs-on, 'ubuntu')
uses: tarantool/setup-tarantool@v1
with:
tarantool-version: ${{ matrix.tarantool }}
tarantool-version: '2.11'

- name: Setup stable tarantool from brew
if: startsWith(matrix.runs-on, 'macos')
run: brew install tarantool

- name: build
run: |
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=${{ matrix.mode }} -DTNTCXX_BUILD_TESTING=ON -DTNTCXX_ENABLE_SSL=OFF ..
cmake -DCMAKE_BUILD_TYPE=${{ matrix.mode }} -DTNTCXX_BUILD_TESTING=ON -DTNTCXX_ENABLE_SSL=ON ..
make -j

- name: test without SSL
if: matrix.test-ssl == 'Disabled'
run: cd build && ctest --output-on-failure -E ClientSSL.test
Copy link
Member

@CuriousGeorgiy CuriousGeorgiy Oct 25, 2023

Choose a reason for hiding this comment

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

[feel free to ignore, just for your knowledge]

Nit: you could use the ${{ matrix.test-ssl == 'Disabled' && 'ClientSSL.test' || ''}} ternary operator syntax (see example here) just like in Lua to simplify the code and have less branches.


- name: test
if: matrix.test-ssl == 'Enabled'
run: cd build && ctest --output-on-failure
1 change: 1 addition & 0 deletions src/Utils/Traits.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
*/

#include <cstddef>
#include <iterator>
#include <tuple>
#include <type_traits>
#include <variant>
Expand Down
1 change: 1 addition & 0 deletions test/EncDecTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

#include <set>
#include <map>
#include <vector>

#include "Utils/Helpers.hpp"
#include "Utils/RefVector.hpp"
Expand Down
60 changes: 60 additions & 0 deletions test/Utils/System.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ set_parent_death_signal(pid_t ppid_before_fork, const char *child_program_name)
}
}

#ifdef __linux__

inline int
launchTarantool(bool enable_ssl = false)
{
Expand All @@ -85,6 +87,64 @@ launchTarantool(bool enable_ssl = false)
exit(EXIT_FAILURE);
}

#else

/**
* Launches intermediate process, connected with parent by pipe.
* The intermediate process lauches another process, running tarantool, and then
* falls asleep on reading from pipe. When parent process is dead, the pipe
* will be closed, and the intermediate process will read EOF. Right after, it
* will kill its child process, which is Tarantool, and exit.
* In the case, when the intermediate process fails to fork Tarantool, it kills
* the parent process and terminates.
*/
Copy link
Member

@CuriousGeorgiy CuriousGeorgiy Oct 23, 2023

Choose a reason for hiding this comment

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

I agree with @alyapunov in the sense that this is code is really sophistacted, but I guess there is no other way to implement this approach on macOS, so I see two ways to make life easier for us:

  1. Since the approach @drewdzzz proposes here is portable, let's use it everywhere: i.e., drop the Linux version and only leave this one.
  2. Abandon this approach entirely, and instead refactor our test code to save the Tarantool PID (perhaps, create some TarantoolInstance object), and clean it up properly when testing fails. An approach like this is used in go-tarantool, for instance 1. Not sure what to do, if the test crashes though (seems like the process leaks in this case), I guess we should consult with someone from @tarantool/ecosystem, like @DifferentialOrange or @0x501D.

Footnotes

  1. https://github.com/tarantool/go-tarantool/blob/bd6aab9db09260abe3bfe929b23e81a0171abf2b/shutdown_test.go#L137-L139

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what to do, if the test crashes though

go-tarantool is also unable to shutdown Tarantool processes in case of a crash now, so this problem is unresolved for us as well.

Choose a reason for hiding this comment

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

Not sure what to do, if the test crashes though

go-tarantool is also unable to shutdown Tarantool processes in case of a crash now, so this problem is unresolved for us as well.

This is an implementation flaw. It could be implemented in Go for panic in fact:
tarantool/go-tarantool#147 (comment)

But there is no solution to the problem in go-tarantool that would be suitable for C++.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you think that a process leak happens?
I crashed the test and all processes have been deleted.

Copy link
Member

@CuriousGeorgiy CuriousGeorgiy Oct 24, 2023

Choose a reason for hiding this comment

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

Why do you think that a process leak happens?

I mean if we go for option 2 that I listed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, kill is signal-safe, so we could kill Tarantool from signal handler.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I also thought about this, we could add signal handlers from recoverable signals (i.e., even go can't do anything about SIGKILL).

Another path we can take is setting up and cleaning up Tarantool externally (i.e., via the test harness or some shell scripting), but it may not be scalable or convenient.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's time for @alyapunov to take the decision how we should proceed here.

inline int
launchTarantool(bool enable_ssl = false)
{
pid_t ppid_before_fork = getpid();
int p[2];
pipe(p);
pid_t pid = fork();
if (pid == -1) {
fprintf(stderr, "Can't launch Tarantool: fork failed! %s",
strerror(errno));
return -1;
}
/* Returning from parent. */
if (pid != 0)
return 0;

/*
* It is necessary to close write end of pipe here if we want to read
* EOF when the parent process is dead.
*/
close(p[1]);
pid = fork();
if (pid == -1) {
/*
* We've already returned OK in the parent, so let's kill it if
* the intermediate process fails to fork Tarantool.
*/
kill(ppid_before_fork, SIGKILL);
exit(EXIT_FAILURE);
}
if (pid == 0) {
close(p[0]);
const char *script = enable_ssl ? "test_cfg_ssl.lua" : "test_cfg.lua";
if (execlp("tarantool", "tarantool", script, NULL) == -1) {
fprintf(stderr, "Can't launch Tarantool: execlp failed! %s\n",
strerror(errno));
kill(getppid(), SIGKILL);
}
}
char buf[1];
read(p[0], buf, 1);
kill(pid, SIGTERM);
exit(0);
}

#endif

inline int
cleanDir() {
pid_t pid = fork();
Expand Down