rpms / rubygem-mysql2

Clone
Source Code
GIT

#13 Testsuite fixes and enhancements
Opened a year ago by mschorm. Modified a year ago
rpms/ mschorm/rubygem-mysql2 testuite_bz-2144488  into  rawhide

Merge
file modified
+27 -17 
@@ -6,7 +6,7 @@ 
 

  

  Name: rubygem-%{gem_name}
 

  Version: 0.5.4
 

- Release: 2%{?dist}
 

+ Release: 3%{?dist}
 

  Summary: A simple, fast Mysql library for Ruby, binding to libmysql
 

  License: MIT
 

  URL: https://github.com/brianmario/mysql2
 
@@ -87,38 +87,44 @@ 
 

  

  mkdir "${MYSQL_TEST_DATA_DIR}"
 

  mysql_install_db \
 

+   --no-defaults \
 

+   --auth-root-authentication-method=normal \
 

    --datadir="${MYSQL_TEST_DATA_DIR}" \
 

    --log-error="${MYSQL_TEST_LOG}"
 

  

  %{_libexecdir}/mysqld \
 

+   --no-defaults \
 

    --datadir="${MYSQL_TEST_DATA_DIR}" \
 

    --log-error="${MYSQL_TEST_LOG}" \
 

    --socket="${MYSQL_TEST_SOCKET}" \
 

    --pid-file="${MYSQL_TEST_PID_FILE}" \
 

    --port="${MYSQL_TEST_PORT}" \
 

-   --ssl &
 

+   &
 

+ 

+ # ^^^ There are also SSL tests in the testuite,
 

+ # but they require the DB server to be executed with SSL support.
 

+ # That is done by adding '--ssl' argument
 

+ #
 

+ # However the DB server will abort, if the SSL isn't configured properly
 

+ # (valid certificates etc.). So far we haven't got capacity to make the SSL working.
 

+ # But their results should be valuable and it should be worth fixing.
 

+ #
 

+ # https://bugzilla.redhat.com/show_bug.cgi?id=2144488
 

  

  for i in $(seq 10); do
 

+   echo "Waiting for the DB server to accept connections ... ${i}"
 

    sleep 1
 

-   if grep -q 'ready for connections.' "${MYSQL_TEST_LOG}"; then
 

+   if grep -q 'ready for connections' "${MYSQL_TEST_LOG}"; then
 

      break
 

+   elif [ $i -eq 10 ]; then
 

+     echo "ERROR: the DB server hasn't come online !"
 

+     echo "       the testuite can't be run !"
 

+     echo "Here follows the DB server log: "
 

+     cat "${MYSQL_TEST_LOG}"
 

+     exit 1
 

    fi
 

-   echo "Waiting connections... ${i}"
 

  done
 

  

- # Reset password for the root user due to MariaDB 10.4 authentication change.
 

- # See https://mariadb.com/kb/en/authentication-from-mariadb-104/#altering-the-user-account-to-revert-to-the-previous-authentication-method
 

- mysql -u ${MYSQL_TEST_USER} \
 

-   -e "ALTER USER 'root'@'localhost' IDENTIFIED VIA mysql_native_password USING PASSWORD('')" \
 

-   -S "${MYSQL_TEST_SOCKET}" \
 

-   -P "${MYSQL_TEST_PORT}"
 

- 

- # See https://github.com/brianmario/mysql2/blob/master/ci/setup.sh
 

- mysql -u root \
 

-   -e 'CREATE DATABASE /*M!50701 IF NOT EXISTS */ test' \
 

-   -S "${MYSQL_TEST_SOCKET}" \
 

-   -P "${MYSQL_TEST_PORT}"
 

- 

  # This GC method call is problematic on ppc64le builders, stalling the tests execution.
 

  # https://github.com/brianmario/mysql2/issues/1261
 

  %ifarch ppc64le
 
@@ -168,6 +174,10 @@ 
 

  

  

  %changelog
 

+ * Thu Dec 08 2022 Michal Schorm <mschorm@redhat.com> - 0.5.4-3
 

+ - Extensive fixes and enhancement to the testuite execution preparations code
 

+   Related: rhbz#2144488
 

+ 

  * Sat Jul 23 2022 Fedora Release Engineering <releng@fedoraproject.org> - 0.5.4-2
 

  - Rebuilt for https://fedoraproject.org/wiki/Fedora_37_Mass_Rebuild

no initial comment

This PR does not contain actual fix for the SSL issue, just a workaround (disabling).

I'd suggest you to add it on top of this PR, instead of inside of this PR.

Build succeeded.

However the DB server will abort, if the SSL isn't configured properly
(valid certificates etc.). So far we haven't got capacity to make the SSL working.
But their results should be valuable and it should be worth fixing.

Thanks for the PR.
What is the actual work to make the SSL properly?

There are 6 commits in the PR. Are all the commits essential to fix the FTBFS?
Edited a year ago by jaruga

What do you think about splitting this PR to 2 PRs? This PR is about an essential change to fix FTBFS. Another PR is for refactoring and enhancements. Because it's hard for me to review the part related to the FTBFS in this PR.

None of the commits fix the FTBFS.
One of the commits workarounds the FTBFS. (removal of the '--ssl' argument)

All of the commits fixes issues there were present in the SPECfile for a longer time.

I've discussed it with @vondruch and rather thoroughly described it in the
https://bugzilla.redhat.com/show_bug.cgi?id=2144488

The actual SSL fix is yours to make.
I offer updates regarding current best practices, as a MariaDB stack maintainer.
Edited a year ago by mschorm

None of the commits fix the FTBFS.
One of the commits workarounds the FTBFS. (removal of the '--ssl' argument)

All of the commits fixes issues there were present in the SPECfile for a longer time.

I've discussed it with @vondruch and rather thoroughly described it in the
https://bugzilla.redhat.com/show_bug.cgi?id=2144488

Thanks for your quick replying. I was using the "fix" as "workaround" from FTBFS (red in Koji) to non-FTBFS (green in Koji). And I confirmed that only the 4th commit "[4/6] Testsuite fix - temporary disabling of the testing of the SSL" is essential to make the Koji non-FTBFS (green).

I see you declare what you would plan to do on the comment 2 today. But I don't see you discussed with Vit on the ticket about what you plan to do. I mean I don't see Vit's response on the ticket. Did you discuss it with Vit on offline or IRC?
https://bugzilla.redhat.com/show_bug.cgi?id=2144488#c2

I don't deny the contents of your commits. You know about Mysql very well.

My point is why other commits have "Related: #2144488" in the commit message, and those are related to the ticket 2144488 "SSL tests for the 'rubygem-mysql2' package are broken"? What is the benefit?

The actual SSL fix is yours to make.

OK. Maybe "Yours" means people working at rpms/ruby.

I offer updates regarding current best practices, as a MariaDB stack maintainer.

OK. Again I am positive to follow the best practices.

For the commit below,

diff --git a/rubygem-mysql2.spec b/rubygem-mysql2.spec
index 4922512..1571e18 100644
--- a/rubygem-mysql2.spec
+++ b/rubygem-mysql2.spec
@@ -99,7 +99,17 @@ mysql_install_db \
   --socket="${MYSQL_TEST_SOCKET}" \
   --pid-file="${MYSQL_TEST_PID_FILE}" \
   --port="${MYSQL_TEST_PORT}" \
-  --ssl &
+  &
+
+# ^^^ There are also SSL tests in the testuite,
+# but they require the DB server to be executed with SSL support.
+# That is done by adding '--ssl' argument
+#
+# However the DB server will abort, if the SSL isn't configured properly
+# (valid certificates etc.). So far we haven't got capacity to make the SSL working.
+# But their results should be valuable and it should be worth fixing.
+#
+# https://bugzilla.redhat.com/show_bug.cgi?id=2144488

If you see other parts in the rubygem-mysql2.spec, you may see how and how much the comments are written. A comment is always above on the target logic. We tend to write more context in the commit message rather than the comment in the spec file. I would like to align with other parts of the comment like below. And I feel that the word "--ssl option" is more proper than the word "--ssl argument".

# TODO: Add `--ssl` option. We dropped the option to skip the SSL tests as a
# temporary workaround.
# https://bugzilla.redhat.com/show_bug.cgi?id=2144488
%{_libexecdir}/mysqld \
  --no-defaults \
  --datadir="${MYSQL_TEST_DATA_DIR}" \
  --log-error="${MYSQL_TEST_LOG}" \
  --socket="${MYSQL_TEST_SOCKET}" \
  --pid-file="${MYSQL_TEST_PID_FILE}" \
  --port="${MYSQL_TEST_PORT}" \
  &

I like to remove the prefix "[N/N] Testsuite fix - " in each commit message. If you see the past commits on rpms/rubygem-mysql2 or rpms/ruby, you see we don't use such a prefix, and you see how the git commit messages look like.
Edited a year ago by jaruga

We have aligned the testing logic with the upstream. There is a merit to do it. Because we can report it to the upstream in a similar testing environment.

If we want to apply these enhancements, it's better to contribute the code to the upstream.
The files in the ci directory are called by GitHub Actions and Travis CI.

For example, the related files are below. There may be other files to modify in the ci directory.
https://github.com/brianmario/mysql2/blob/master/ci/setup.sh
https://github.com/brianmario/mysql2/blob/master/ci/setup_container.sh

@jaruga if you are going to take care about this PR (appreciated :thumbsup:), the idea was to review each issue as described in the BZ, analyze it together with the proposed fixes, submit upstream if needed and apply what is suitable.

E.g. I'd very much like to see the SSL test cases fixed instead of workarounding them. But we might choose to land the workaround, because beginning of year, the mass rebuild for Ruby 3.2.0 needs to happen.

And yes, because I was reporter of the rubygem-mysql2 FTBFS after MariaDB update, we had some discussions with @mschorm via IRC / GMeet, so not everything is recorded in the BZ ticket or here. Sorry about that.

But I don't see you discussed with Vit on the ticket about what you plan to do. I mean I don't see Vit's response on the ticket. Did you discuss it with Vit on offline or IRC?
https://bugzilla.redhat.com/show_bug.cgi?id=2144488#c2

I agreed with Vít on that those updates makes sense.
Let's the discussion in this PR be place of decision whether and how they are actually merged.

My point is why other commits have "Related: #2144488" in the commit message, and those are related to the ticket 2144488 "SSL tests for the 'rubygem-mysql2' package are broken"? What is the benefit?

Oh. That's something that slipped my mind, beacuse, originally, the BZ summary was
"MariaDB 10.5.18 seems to break rubygem-mysql2"
and after thorough investigation of this vaguely named problem I wrote the comment 2 in BZ, changed the summary to something I saw more fitting "SSL tests for the 'rubygem-mysql2' package are broken" and assigned it back to rubygem-mysql2 pkg maintainers.

Now it doesn't seem like an ideal decision.
Maybe we should create a new BZ dedicated to making the SSL tests work, and keep this BZ to everything else around I found on the way.

The actual SSL fix is yours to make.

OK. Maybe "Yours" means people working at rpms/ruby.

Yes, I definitely meant rubygem-mysql2 package maintainers in general.
There are 2 package members, one of them is '@ruby-packagers-sig'. Both you and Vit are part of thsi SIG - I presume. It's not very transparent who the members are.

Work I'll take a look at

(1) Putting comment about the SSL option above the code

(2) using word 'option' instead of 'argument' in the description

Work to be discussed:

(3)

I like to remove the prefix "[N/N] Testsuite fix - " in each commit message. If you see the past commits on rpms/rubygem-mysql2 or rpms/ruby, you see we don't use such a prefix, and you see how the git commit messages look like.

I use this prefix to mark, that several different commits are part of a bigger whole.
I want to use "single change - single commit" approach, but since solving of a single problem might consist of multiple changes, I feel a need to mark the fact somehow.

Do you still want me to remove the prefixes ?

(4)

We have aligned the testing logic with the upstream.
There is a merit to do it.
Because we can report it to the upstream in a similar testing environment.

If we want to apply these enhancements, it's better to contribute the code to the upstream.
The files in the ci directory are called by GitHub Actions and Travis CI.

For example, the related files are below. There may be other files to modify in the ci directory.
https://github.com/brianmario/mysql2/blob/master/ci/setup.sh
https://github.com/brianmario/mysql2/blob/master/ci/setup_container.sh

I have no doubt that aligning the testing logic with upstream brings benefits to you.

However I am not familiar with any of the following: Ruby, rubygem-mysql2, github actions, rubygem-mysql2 upstream testing.

I took the effort to understand the non-ruby testing you do in the Fedora SPECfile.
I described the issues I found and the enhancements I propose.
I created the PR, so you can see what precise code changes I mean by the suggestions I wrote into the BZ.

I don't have the capacity to dive into this package further.

It is not clear to me now if you would like to merge this PR (once updated), or rather bring all of the changes to the upstream right away.
If there wouldn't be a will to merge this PR at all, I would like not to invest more time updating the commits, and rather leave it as it is instead, that's all.

So, should I do the updates (1), (2), (3) ?

Thanks for explaining it! So far I read the (1), (2), (3).

So, should I do the updates (1), (2), (3) ?

Yes, I would like you to do it.

(3)

I use this prefix to mark, that several different commits are part of a bigger whole.
I want to use "single change - single commit" approach, but since solving of a single problem might consist of multiple changes, I feel a need to mark the fact somehow.

Do you still want me to remove the prefixes ?

My concern is the "a bigger whole", recognizing some commits as one group is subjective. Before and after this PR, there were or will be some commits related to enhancing testing logic. I am considering the case that only the 4th commit "[4/6] Testsuite fix - temporary disabling of the testing of the SSL" will be backported to a downstream rubygem-mysql2 package just to make FTBFS green in Koji. And the commit message is weird in the downstream.
Edited a year ago by jaruga

It is not clear to me now if you would like to merge this PR (once updated), or rather bring all of the changes to the upstream right away.
If there wouldn't be a will to merge this PR at all, I would like not to invest more time updating the commits, and rather leave it as it is instead, that's all.

I am positive to merge this PR. in my understanding, following the "upstream first", we try to contribute to the upstream first, at least we open an issue ticket on the upstream, then if we see the downstream specific features are useful to apply soon, we apply it with the upstream issue or PR link in the spec file.

I am not sure why you assumed "there wouldn't be a will to merge this PR at all".
Edited a year ago by jaruga

Yesterday, I had an opportunity to discuss this topic with @mschorm (Michal) in person. I try to share the context here in my words. If I am wrong, please correct me.

In my understanding, Michal thought that this PR was intended to share and raise the issues that he found. He expected some feedback in the review process of the PR. But he didn't intend to have extra effort to work to modify and rebase the PR.

I think I want to utilize this PR's content. So, I suggested I will cherry-pick the commits on the PR and send another PR with small chunk to rpms/rubygem-mysql2 repository. And he agreed on it. And he is happy to help a code review for the PR. And I am also planning to contribute Michal's code to the upstream. And when I need to discuss with the upstream about the code in a better way, he is happy to help.
Edited a year ago by jaruga

I sent a PR to fix the SSL issue. Fortunately I was able to find a solution to fix it. And I plan to sent another PR to enhance the tests in rubygem-mysql2 by using the content of this PR.
Edited a year ago by jaruga

@mschorm would you mind to rebase this PR now that the SSL test cases were fixed by #14

@mschorm would you mind to rebase this PR now that the SSL test cases were fixed by #14

IOW the patch 4 should not be needed anymore, while we still need to consider (and possibly upstream) the rest.

I opened another PR based on the patch 5 ([5/6] Testsuite fix - enhance the error handling of the loop that checks whether the DB server is alive).
https://src.fedoraproject.org/rpms/rubygem-mysql2/pull-request/15
Metadata
None
No Tags
Changes Summary 1
+27 -17
file changed
rubygem-mysql2.spec
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.