-
Notifications
You must be signed in to change notification settings - Fork 72
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
Remove the GPL as an alternative licence #119
Remove the GPL as an alternative licence #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is gnu_parallel really just parallel or have there been modifications to it? I am a little leary of making the change without that confirmation.
For ease of merging later, I would also suggest splitting this into one or two additional PRs. One PR to replace the perl with sed; one to use $PARALLEL instead of gnu_parallel or parallell; and the one you are actually wanting to remove gnu_parallel. The first two are upstreamable and would make it easier for merging later.
diff --git a/build_tools/gnu_parallel b/build_tools/gnu_parallel
index abbf8f100..757b25f11 100755
--- a/build_tools/gnu_parallel
+++ b/build_tools/gnu_parallel
@@ -1561,6 +1561,7 @@ sub save_stdin_stdout_stderr {
::die_bug("Can't dup STDERR: $!");
open $Global::original_stdin, "<&", "STDIN" or
::die_bug("Can't dup STDIN: $!");
+ $Global::is_terminal = (-t $Global::original_stderr) && !$ENV{'CIRCLECI'} && !$ENV{'TRAVIS'};
}
sub enough_file_handles {
@@ -1840,12 +1841,17 @@ sub start_another_job {
}
}
+$opt::min_progress_interval = 0;
+
sub init_progress {
# Uses:
# $opt::bar
# Returns:
# list of computers for progress output
$|=1;
+ if (not $Global::is_terminal) {
+ $opt::min_progress_interval = 30;
+ }
if($opt::bar) {
return("","");
}
@@ -1870,6 +1876,9 @@ sub drain_job_queue {
}
my $last_header="";
my $sleep = 0.2;
+ my $last_left = 1000000000;
+ my $last_progress_time = 0;
+ my $ps_reported = 0;
do {
while($Global::total_running > 0) {
debug($Global::total_running, "==", scalar
@@ -1880,14 +1889,38 @@ sub drain_job_queue {
close $job->fh(0,"w");
}
}
- if($opt::progress) {
+ # When not connected to terminal, assume CI (e.g. CircleCI). In
+ # that case we want occasional progress output to prevent abort
+ # due to timeout with no output, but we also need to stop sending
+ # progress output if there has been no actual progress, so that
+ # the job can time out appropriately (CirecleCI: 10m) in case of
+ # a hung test. But without special output, it is extremely
+ # annoying to diagnose which test is hung, so we add that using
+ # `ps` below.
+ if($opt::progress and
+ ($Global::is_terminal or (time() - $last_progress_time) >= 30)) {
my %progress = progress();
if($last_header ne $progress{'header'}) {
print $Global::original_stderr "\n", $progress{'header'}, "\n";
$last_header = $progress{'header'};
}
- print $Global::original_stderr "\r",$progress{'status'};
- flush $Global::original_stderr;
+ if ($Global::is_terminal) {
+ print $Global::original_stderr "\r",$progress{'status'};
+ }
+ if ($last_left > $Global::left) {
+ if (not $Global::is_terminal) {
+ print $Global::original_stderr $progress{'status'},"\n";
+ }
+ $last_progress_time = time();
+ $ps_reported = 0;
+ } elsif (not $ps_reported and (time() - $last_progress_time) >= 60) {
+ # No progress in at least 60 seconds: run ps
+ print $Global::original_stderr "\n";
+ system("ps", "-wf");
+ $ps_reported = 1;
+ }
+ $last_left = $Global::left;
+ flush $Global::original_stderr;
}
if($Global::total_running < $Global::max_jobs_running
and not $Global::JobQueue->empty()) {
@@ -1921,7 +1954,7 @@ sub drain_job_queue {
not $Global::start_no_new_jobs and not $Global::JobQueue->empty());
if($opt::progress) {
my %progress = progress();
- print $Global::original_stderr "\r", $progress{'status'}, "\n";
+ print $Global::original_stderr $opt::progress_sep, $progress{'status'}, "\n";
flush $Global::original_stderr;
}
}
@@ -1954,10 +1987,11 @@ sub progress {
my $eta = "";
my ($status,$header)=("","");
if($opt::eta) {
- my($total, $completed, $left, $pctcomplete, $avgtime, $this_eta) =
- compute_eta();
- $eta = sprintf("ETA: %ds Left: %d AVG: %.2fs ",
- $this_eta, $left, $avgtime);
+ my($total, $completed, $left, $pctcomplete, $avgtime, $this_eta) =
+ compute_eta();
+ $eta = sprintf("ETA: %ds Left: %d AVG: %.2fs ",
+ $this_eta, $left, $avgtime);
+ $Global::left = $left;
}
my $termcols = terminal_columns();
my @workers = sort keys %Global::host;
I separated the changes in this PR into two commits exactly for this reason. The first commit only deals with cleanup of the Makefile and removal of the dependency on Perl, and the second one deals with the removal of the vendored GNU Parallel and the licence. |
0aedb0c
to
3a14200
Compare
3a14200
to
badb049
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. Do the Circle or related jobs need to change to install parallel?
Can you post into the PR comments what happens on a machine in which parallel is not installed and if PARALLEL points to something off?
@@ -988,7 +997,7 @@ valgrind_check_0: gen_parallel_tests | |||
| $(prioritize_long_running_tests) \ | |||
| grep -E '$(tests-regexp)' \ | |||
| grep -E -v '$(valgrind-exclude-regexp)' \ | |||
| parallel -j$(J) --plain --joblog=LOG --eta --gnu \ | |||
| "$(PARALLEL)" -j$(J) --plain --joblog=LOG --eta --gnu \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens here if PARALLEL is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll get a runtime error, but this is OK because the valgrind_check_0
target is an internal target that's only called from valgrind_check
if PARALLEL_OK
is set to 1
.
Makefile
Outdated
[ "$(J)" -gt 1 ]) \ | ||
|| !command -v parallel 2>&1 >/dev/null \ | ||
|| !(parallel --gnu --version 2>/dev/null | \ | ||
grep -q 'Ole Tange'); \ | ||
.PHONY: assert_parallel_ok | ||
assert_parallel_ok: | ||
$(AM_V_GEN)if [ "$(PARALLEL_OK)" != "1" ]; \ | ||
then \ | ||
echo "Need to have GNU Parallel and J > 1"; exit 1; \ | ||
fi; \ | ||
echo >&2 "Could not find GNU Parallel. Please set the PARALLEL variable." && false; \ | ||
fi | ||
$(AM_V_GEN)if !(case "$(J)" in [!0-9]*|"") false;; esac && \ | ||
[ "$(J)" -gt 1 ]); \ | ||
then \ | ||
echo >&2 "Need to have J > 1 (currently: $(J))" && false; \ | ||
fi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it OK for PARALLEL not to be set if J=1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the parloop
and parallel_check
(which depend on this new target) this wasn't OK. The original code did the following in that case:
echo "Need to have GNU Parallel and J > 1"; exit 1;
However, with the latest changes this isn't relevant anymore, since those two targets were removed.
Makefile
Outdated
assert_parallel_ok: | ||
$(AM_V_GEN)if [ "$(PARALLEL_OK)" != "1" ]; \ | ||
then \ | ||
echo >&2 "Could not find GNU Parallel. Please set the PARALLEL variable." && false; \ | ||
fi | ||
$(AM_V_GEN)if !(case "$(J)" in *[!0-9]*|"") false;; esac && \ | ||
[ "$(J)" -gt 1 ]); \ | ||
then \ | ||
echo >&2 "Need to have J > 1 (currently: $(J))" && false; \ | ||
fi; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems off to me. Shouldn't we only care about PARALLEL_OK if J > 1 ? What if J is not set or == 1 does PARALLEL still need to be installed (and PARALLEL_OK == 1)?
Makefile
Outdated
assert_parallel_ok: | ||
$(AM_V_GEN)if [ "$(PARALLEL_OK)" != "1" ]; \ | ||
then \ | ||
echo >&2 "Could not find GNU Parallel. Please set the PARALLEL variable." && false; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set PARALLEL or install GNU parallel?
4d280eb
to
f6ca371
Compare
We don't use CircleCI at the moment, and our GitHub Actions workflows already install GNU Parallel as part of the setup for jobs.
After the latest changes (pulling the upstream Makefile cleanup commit, which removes the targets that error out in case parallel isn't present) we won't get any error in either case, and the tests will simply run serially instead of in parallel (that's the behaviour of the original code). Here's how the first few lines of the output look on my machine when setting
|
2c5f492
to
7efa65d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running with the Make changes, I find that if PARALLEL is set to something that does not exist or if parallel itself cannot be found, there are no errors printed -- the tests just run serially. If this is the intent, the PR is good to go (please add a comment somewhere as such). If not, the PR can still be committed if the issue is raised/addressed in a separate issue/PR.
This is indeed the intended behaviour, as I noted in my previous comment. The original comment above the
|
This picks the "Clean up variables for temporary directory" commit (e03d958) from RocksDB for easier upstreaming and merging in the future. Summary: Having all of TMPD, TMPDIR and TEST_TMPDIR as configuration parameters is confusing. This change simplifies a number of things by standardizing on TEST_TMPDIR, while still recognizing the old names also. In detail: * crash_test.mk also needs to use TEST_TMPDIR for crash test, so put in shared common.mk (an upgrade of python.mk) * Always exporting TEST_TMPDIR eliminates the need to propagate TMPD or export TEST_TMPDIR in selective places. * Use --tmpdir option to gnu_parallel so that it doesn't need TMPDIR environment variable * Remove obsolete parloop and parallel_check Makefile targets * Remove undefined, unused function ResetTmpDirForDirectIO() Pull Request resolved: facebook/rocksdb#9961 Test Plan: manual + CI Reviewed By: riversand963 Differential Revision: D36212178 Pulled By: pdillinger fbshipit-source-id: b76c1876c4f4d38b37789c2779eaa7c3026824dd
Remove dependency on perl, which isn't really needed and can be replaced with POSIX tools. Also do some minor cleanup of the check targets.
This allows using more up to date versions from the user's system as well as possibly patched versions with additional features.
We only license Speedb under the Apache 2.0 licence, so we need to remove the GPL text from the repository. Since GNU Parallel is currently vendored in the repo and requires having a copy of the GPL text, remove it as well and depend on the system installed binary if exists (out build instructions ask users to install it anyway, and it's installed in CI).
7efa65d
to
bd6a5a9
Compare
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
The changes in #119 included an upstream commit that moved the TEST_TMPDIR setting to common.mk and executed it unconditionally. This is causing pollution of /dev/shm unnecessarily, and also causes the crash tests and other targets to use /dev/shm when they originally didn't. On machines with small /dev/shm this leads to the crash tests failing with an out of space error. Make the setting of TEST_TMPDIR and the creation of a temporary directory under it execute only once during a single make invocation and only for the check target, as it was originally. This requires dealing with make restarts (due to makefiles getting remade) in the form of adding another temporary configuration file named test_config.mk
This PR removes the GPL as an alternative licence since we only license Speedb under the Apache 2.0 licence. This requires removing the bundled GNU Parallel as well because it requires having the licence text along for the ride, so we adjust the Makefile to depend on the system-installed binary instead and fall back to running tests serially if not found.
While at it, clean up the Makefile a bit and remove the unneeded dependency on Perl for things that can easily be done using POSIX tools.
Test Plan: run
make check
and other targets that use/depend on GNU Parallel.