rpms / rubygem-pg

Clone
Source Code
GIT

#3 Assign random testing port
Merged 4 years ago by jaruga. Opened 4 years ago by jaruga.
rpms/ jaruga/rubygem-pg feature/wait-to-clean-up  into  master

file modified
+5 -1 
@@ -80,9 +80,13 @@ 
 

  pushd .%{gem_instdir}
 

  # Set --verbose to show detail log by $VERBOSE.
 

  # See https://github.com/ged/ruby-pg/blob/master/spec/helpers.rb $VERBOSE
 

- if ! ruby -S --verbose \
 

+ # Assign a random port to consider a case of multi builds in parallel in a host.
 

+ # https://github.com/ged/ruby-pg/pull/39
 

+ if ! PGPORT="$((54321 + ${RANDOM} % 1000))" ruby -S --verbose \
 

    rspec -I$(dirs +1)%{gem_extdir_mri} -f d spec; then
 

+   echo "==== [setup.log start ] ===="
 

    cat tmp_test_specs/setup.log
 

+   echo "==== [setup.log end ] ===="
 

    false
 

  fi
 

  popd

This PR is to improve a maintainability of rubygem-pg.
2 improvements.

Waiting to stop a testing postgresql daemon

The problem is below error message sometimes happens in build.log.
That's because previous build on the mock does not close the daemon on the port "54321" effectively.
"54321" is testing post used by only rubygem-pg.
The mock process finishes before the daemon is stopped.

build.log

waiting for server to start....2019-05-13 16:25:07.565 UTC [2106] LOG:  could not bind IPv6 address "::1": Address already in use
2019-05-13 16:25:07.565 UTC [2106] HINT:  Is another postmaster already running on port 54321? If not, wait a few seconds and retry.
2019-05-13 16:25:07.565 UTC [2106] LOG:  could not bind IPv4 address "127.0.0.1": Address already in use

So, I added the logic to wait to stop daemon. It's like rubygem-mysql2.
But in case of rubygem-pg, the closing logic is in the spec/helpers.rb.
https://src.fedoraproject.org/rpms/rubygem-mysql2/blob/master/f/rubygem-mysql2.spec#_157

Add marking lines at the start and end of the setup.log

2nd improvement is to add marking lines at the start and end of the setup.log.
Because set -x does not show the log by order in mock.log.

Below is the logic to show the content of the setup.log.
But in build.log, the + cat tmp_test_specs/setup.log is shown after showing the stdout of the content.
So, ==== [setup.log start ] ==== and ==== [setup.log end ] ==== are important to know the actual content.

+ cat tmp_test_specs/setup.log

build.log

==== [setup.log start ] ====
The files belonging to this database system will be owned by user "mockbuild".
This user must also own the server process.
The database cluster will be initialized with locale "C".
The default text search configuration will be set to "english".
...
Is server running?
==== [setup.log end ] ====
RPM build errors:
+ echo '==== [setup.log start ] ===='
+ cat tmp_test_specs/setup.log
+ echo '==== [setup.log end ] ===='
+ false

Wouldn't just wait (shell wait) be sufficient?
You could also do kill $(cat tmp_test_specs/data/postmaster.pid) or wait $(cat tmp_test_specs/data/postmaster.pid) to be more specific.

@pvalena thanks for the info! I did not know wait $(cat foo.pid) command.

The tmp_test_specs/data/postmaster.pid is not like normal pid file including only pid.
Below is the actual content.
Maybe we can not do your ways.

$ cat tmp_test_specs/data/postmaster.pid 
19819
/home/jaruga/git/ruby-pg/tmp_test_specs/data
1557785047
54321
/home/jaruga/git/ruby-pg/tmp_test_specs
localhost
 54321001  38469642
ready

As without this modification, I failed the rubygem-pg build around 10 times continously in RHEL8, I like this PR is merged :)

kill $(cat tmp_test_specs/data/postmaster.pid) is not suitable in this case.
Because the logic in the spec/helper.rb during the process of rspec kills postgresql daemon. It's not good idea to disturb the killing process by manual kill command outside of rspec process.

The process in the rspec executes the command to stop the postgresql daemon. Then the daemon is asynchronously stopped separating from the rspec process.
Edited 4 years ago by jaruga

Besides that @pvalena's suggestion can be modified to use head, sed or any other command to extract the PID form the pid file, you don't provide enough evidence what the problem actually is (where is the relevant part of log, which would demonstrate when/where the problem happens?) nor evidence that your proposal solves anything (the scratch build would likely succeed without the patch).

wait without PID will wait for all "children processes", which is what you usually want. From help:

If n is not given, all currently active child processes are waited for, and the return status is zero.

Note that it's a bash builtion, not a linux command.

I agree \w vondruch that it should end regularly and wait for it (not async).
This can be achieved (in shell) f.e. with: 

trap 'kill 0; exit' ERR EXIT

(from the top of my head)

For trap see here.

And kill 0 simply kills all 'child processes'.
This can be probably done in Ruby in some way (as a suggestion for upstream).

rebased onto d508830
4 years ago

I created 2 commits for this PR.
I also canceled "Waiting to stop a testing postgresql daemon" way.
Instead of that, I added random testing port feature.

On the build system for RHEL, there is a case.
x86_64 and i686's build are run on a same host, and (maybe) different directory.
As a result, each tests use same testing port to start PostgreSQL from spec/helpers.rb.

Then the log file shows an error. That was the reason.
https://github.com/ged/ruby-pg/pull/39

So, if Koji on Fedora also can have this case, this commit improves it.

Scratch build: ok https://koji.fedoraproject.org/koji/taskinfo?taskID=34871509

Scratch build to conform the actually assigned port with below modification :
https://koji.fedoraproject.org/koji/taskinfo?taskID=34871636

$ git diff
diff --git a/rubygem-pg.spec b/rubygem-pg.spec
index df6c794..ad39217 100644
--- a/rubygem-pg.spec
+++ b/rubygem-pg.spec
@@ -91,6 +91,7 @@ if ! ruby -S --verbose \
   echo "==== [setup.log end ] ===="
   false
 fi
+cat tmp_test_specs/setup.log
 popd

 %files

You can see different post (54365 and 55145) is assigned for each other.

x86_64: https://kojipkgs.fedoraproject.org//work/tasks/1637/34871637/build.log

    pg_ctl -D /builddir/build/BUILD/pg-1.1.4/usr/share/gems/gems/pg-1.1.4/tmp_test_specs/data -l logfile start
waiting for server to start....2019-05-15 14:17:25.440 UTC [16489] LOG:  listening on IPv6 address "::1", port 54365
2019-05-15 14:17:25.440 UTC [16489] LOG:  listening on IPv4 address "127.0.0.1", port 54365

i686: https://kojipkgs.fedoraproject.org//work/tasks/1640/34871640/build.log

   pg_ctl -D /builddir/build/BUILD/pg-1.1.4/usr/share/gems/gems/pg-1.1.4/tmp_test_specs/data -l logfile start
waiting for server to start....2019-05-15 14:17:26.284 UTC [16759] LOG:  listening on IPv6 address "::1", port 55145
2019-05-15 14:17:26.284 UTC [16759] LOG:  listening on IPv4 address "127.0.0.1", port 55145

LGTM.

Just one additional thing:

Shouldn't be the patch [for pg port] applied in %check instead?

https://github.com/ged/ruby-pg/pull/39

This is just mirror. Not sure if it won't be better to open PR at official repo:

https://bitbucket.org/ged/ruby-pg/

Just one additional thing:
Shouldn't be the patch [for pg port] applied in %check instead?

Wouldn't be better to don't patch at all and just modify the PGPORT env variable, e.g.:

PGPORT=$((54321 + $(od -An -N1 -i /dev/urandom))) rspec spec

This is just mirror. Not sure if it won't be better to open PR at official repo:

I already conformed a ruby-pg member that GitHub is okay to send pull-request in a past time.
For example, current master's latest commit is my commit merged from GitHub pull-request.

Because primary repo is not Git repo but Mercurial repo.
I am not good at using Mercurial (hg command).

Wouldn't be better to don't patch at all and just modify the PGPORT env variable, e.g.:
PGPORT=$((54321 + $(od -An -N1 -i /dev/urandom))) rspec spec

It might be better, because the upstream PR does not have to be merged. though readability is lower than ruby's one.

I adapt your way.
Edited 4 years ago by jaruga

Using bash's $RANDOM is better, isn't it? Simpler and readable.

Below way's ${RANDOM} % 1000 returns a number between 0 and 999.

$ echo "$((54321 + ${RANDOM} % 1000))"
54971
PGPORT="$((54321 + ${RANDOM} % 1000))" rspec spec

https://stackoverflow.com/questions/8988824/generating-random-number-between-1-and-10-in-bash-shell-script#8988890
https://stackoverflow.com/questions/42004870/seed-for-random-environment-variable-in-bash#42005055

Using bash's $RANDOM is better, isn't it? Simpler and readable.
Below way's ${RANDOM} % 1000 returns a number between 0 and 999.
$ echo "$((54321 + ${RANDOM} % 1000))"
54971

PGPORT="$((54321 + ${RANDOM} % 1000))" rspec spec

https://stackoverflow.com/questions/8988824/generating-random-number-between-1-and-10-in-bash-shell-script#8988890
https://stackoverflow.com/questions/42004870/seed-for-random-environment-variable-in-bash#42005055

Yes, setting PGPORT itself a far better solution IMO.
I also think using $RANDOM is better.
Edited 4 years ago by pvalena

2 new commits added

  • Assign random testing port.
  • Add marking lines at the start and end of the setup.log
4 years ago

Rebased.

Now scratch build is success.
https://koji.fedoraproject.org/koji/taskinfo?taskID=34882522
You can see different port is assigned for each build in the bottom part of the build.log

The scratch build is with 

$ git diff
diff --git a/rubygem-pg.spec b/rubygem-pg.spec
index df6c794..ad39217 100644
--- a/rubygem-pg.spec
+++ b/rubygem-pg.spec
@@ -91,6 +91,7 @@ if ! ruby -S --verbose \
   echo "==== [setup.log end ] ===="
   false
 fi
+cat tmp_test_specs/setup.log
 popd

 %files

Good, LGTM.

2 new commits added

  • Assign random testing port.
  • Add marking lines at the start and end of the setup.log
4 years ago

There is manual page to explain $RANDOM. It would be better if there is the link to the upstream issue instead, for the reference, why this is done.

Please remove the RHEL reference from the commit message, that is not helpful for Fedora nor important. Also, it would be nice to put there the upstream ticket for the reference. Otherwise LGTM

2 new commits added

  • Assign a random testing port.
  • Add marking lines at the start and end of the setup.log
4 years ago

I rebased as you said.
As I did not the $RANDOM until yesterday, I thought explaining it is a bash's reserved variable on the comment.
Anyway, I am fine.
Edited 4 years ago by jaruga

Pull-Request has been merged by jaruga
4 years ago
Changes Summary 1
+5 -1
file changed
rubygem-pg.spec
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.