This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=109894600

Taking this review.

Hello Neal, thanks.

-----

Ss a part of this Fedora Self-contained Change:
  https://src.fedoraproject.org/fork/mschorm/rpms/mysql/commits/rawhide_F40_change
I would like to rename package 'community-mysql' to package 'mysql'.

In this Rel-Eng ticket, I explained the history of the two packages:
  https://pagure.io/releng/issue/11819#comment-886942

All of the changes between current rawhide 'community-mysql' and the current rawhide 'mysql' are developed and stored here:
  https://src.fedoraproject.org/fork/mschorm/rpms/mysql/commits/rawhide_F40_change

-----

I have ran the 'fedora-review' myself.
Please note, that because of the vast number of files (over 30.000), and size, it was running for several hours (>2.5), required *a lot* of disk and memory space and the results has over four gigabytes in size.

In the meanwhile, I've tried to fix the .rpmlintrc allow-list, but it seems it is still being ignored. Not sure if I'm doing something wrong, or whether the 'fedora-review' tool ignores it.

I've uploaded the result from 'fedora-review' I've got for a reference here:
  https://mschorm.fedorapeople.org/rhbz_2252763/review_v1.txt

Hi Neal, how's the review going ?

Initial spec review:

> # Set explicit conflicts with 'mariadb' packages
> %bcond_without conflicts_mariadb
> # Provide explicitly the 'community-mysql' names
> #   'community-mysql' names are deprecated and to be removed in future Fedora
> #   but we're leaving them here for compatibility reasons
> %bcond_without provides_community_mysql
> # Obsolete the package 'community-mysql' and all its sub-packages
> %bcond_without obsoletes_community_mysql

Do you need these to be conditionals? It makes it confusing to read the spec and I don't know why you wouldn't have these conflicts and obsoletes enabled?

Also, in general for bconds, is there a reason you're not using the new style? e.g. "%bcond foo 1" for enabled and "%bcond foo 0" for disabled. This is even supported in RHEL 9.

> Provides:         bundled(boost) = %{boost_bundled_version}

Why is boost bundled? Is there no way to use the system version?

> # arm build ends with out of memory error for LTO enabled build
> %ifarch %arm
> %define _lto_cflags %{nil}
> %endif

Can you change this to %arm32? That way it's more clear this is about 32-bit ARM and not all ARM architectures.

> cmake -B %{_vpath_builddir} -LAH

Could you please add a comment that this is about listing all the CMake variables set? It was not immediately obvious to me.

> %if %{without clibrary}

RPM upstream prefers the usage of "%if ! %{with foo}" over using "%if %{without foo}"

From: https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html

"Always test for the with-condition, not the without-counterpart!"

(In reply to Neal Gompa from comment #5)
> > Provides:         bundled(boost) = %{boost_bundled_version}
> 
> Why is boost bundled? Is there no way to use the system version?

I've found that it dates back to 2016 and it might require some reasonable effort to investigate whether it's possible to unbundle, so I created a separate ticket for that, so we don't block this review with that: https://bugzilla.redhat.com/show_bug.cgi?id=2260138

Hope it works for you, Neal :)

(In reply to Honza Horak from comment #6)
> (In reply to Neal Gompa from comment #5)
> > > Provides:         bundled(boost) = %{boost_bundled_version}
> > 
> > Why is boost bundled? Is there no way to use the system version?
> 
> I've found that it dates back to 2016 and it might require some reasonable
> effort to investigate whether it's possible to unbundle, so I created a
> separate ticket for that, so we don't block this review with that:
> https://bugzilla.redhat.com/show_bug.cgi?id=2260138
> 
> Hope it works for you, Neal :)

Yes, works for me! Just need my other feedback addressed now. :)

Hi Neil,

Thanks for all of the catches, I've addressed all of them in my fork [1].

Would you please mind taking a look at the added commits? If there are no more question marks from you, we can move forward with renaming the package.

PS: I haven't renamed the package yet, but I will use the same Obsoletes/Provides as Michal used in his mysql specfile mentioned in the bug description. Only that I will use the preferred conditions and change the obsolete version to the current one (8.0.35-4)

[1] https://src.fedoraproject.org/fork/ljavorsk/rpms/community-mysql/commits/prepare_for_renaming

The changes look good. Now I just need an up to date spec from Michal in here to approve.

Unfortunately the fedorapeople is down, so I'm attaching the specfile to the files in this BZ.

I've also added the version to all Provides, and added the arch-specific provides as well (it was recommended to me when I was doing other renaming Fedora Changes, so I stuck with it)

Created attachment 2011350 [details]
MySQL specfile

You can also see the changes in my fork, it's nicely highlighted: https://src.fedoraproject.org/fork/ljavorsk/rpms/community-mysql/c/6f5af41cc3b49d54fc7a35911ae9fadd49599daa?branch=prepare_for_renaming

Hi Neal, gentle ping

community-mysql should use %version-%release since this is package rename, rather than a fixed version...

Created attachment 2014162 [details]
MySQL specfile

I've added the release to the Provides as well.

Can you please upload this somewhere and make a SRPM like you did for the original bug comment? That way the automation works and I can easily look at it.

Hi Neal, sorry due to the short deadlines for Fedora 40 changes we had to move forward without this review. We had multiple members of our team review it, and we've been able to finish it before the deadlines.

The resorting package mysql8.0 was created and has already replaced community-mysql in Fedora 40.

We can close this Bugzilla, and again, please accept our apology, but we had to make a quick decision (however it didn't affect the overall quality)

Hey. There was some discussion about this in todays FESCo meeting ( https://meetbot.fedoraproject.org/meeting_matrix_fedoraproject-org/2024-03-04/fesco.2024-03-04-19.30.log.html ).

Many folks felt that this shouldn't just be added as a compat package as there's more changes here than would be allowed for that. 

Of course the package is in now, but you should continue the review and fold in any changes from it.

I'm taking over the baton and I'll continue to work on this ticket.

---

The package revieved is now 'mysql8.0'. Branch Rawhide.
  https://src.fedoraproject.org/rpms/mysql8.0

---

Neil's findings so far:

comment 5 - bconds
  FIXED IN: https://src.fedoraproject.org/rpms/mysql8.0/c/04529e24b2aa0258ee03985bf28ff3a0a5e0ee99?branch=rawhide

comment 5 - bundled boost
  SPLIT TO STANDALONE BZ - NOT YET ADDRESSED: https://bugzilla.redhat.com/show_bug.cgi?id=2260138

comment 5 - LTO on %arm32
  FIXED IN: https://src.fedoraproject.org/rpms/mysql8.0/c/78063eb661ca5a0520d0b53bf0d051679fb2f935?branch=rawhide

comment 5 - cmake -B %{_vpath_builddir} -LAH
  FIXED IN: https://src.fedoraproject.org/rpms/mysql8.0/c/c8aa87d9307bab6fcdadae63f9e5a81df943b3f1?branch=rawhide

comment 5 - use "%if ! %{with foo}" instead "%if %{without foo}"
  FIXED IN: https://src.fedoraproject.org/rpms/mysql8.0/c/16b5eee119ea9a22d1f0a2810248c302fa95aeb4?branch=rawhide

---

# Using Pagure URL frozen to the current latest Rawhide commit
Spec URL: https://src.fedoraproject.org/rpms/mysql8.0/raw/af786c40e299c6fb2356f141cb0ef7588b594e74/f/mysql8.0.spec

SRPM URL: https://mschorm.fedorapeople.org/mysql8.0-8.0.36-4.fc41.src.rpm

---

(1) Did I miss any finding ?
(2) Are those URLs what you need ?

I'll take a look at the bundled boost in the meanwhile.

> comment 5 - LTO on %arm32
>   FIXED IN: https://src.fedoraproject.org/rpms/mysql8.0/c/78063eb661ca5a0520d0b53bf0d051679fb2f935?branch=rawhide

I think you could just drop this entirely? 32-bit ARM hasn't been a thing since Fedora 37 (i.e. available on no current branch of Fedora):
https://fedoraproject.org/wiki/Changes/RetireARMv7

> I think you could just drop this entirely? 32-bit ARM hasn't been a thing
> since Fedora 37 (i.e. available on no current branch of Fedora):
> https://fedoraproject.org/wiki/Changes/RetireARMv7

Right.
I dropped the other 32-bit ARM related bits as well:
  https://src.fedoraproject.org/rpms/mysql8.0/c/cc8a02280b10b98a586f60ed975bb06590f15614?branch=rawhide

You should also drop everything related to s390, we don't offer that 32(ish) bit architecture anymore.

(In reply to Neal Gompa from comment #23)
> You should also drop everything related to s390, we don't offer that 32(ish) bit architecture anymore.

Okay.

I also noticed, I use the '%{arm}' macro on few places.
  | rpm --eval %{arm}
  | armv3l armv4b armv4l armv4tl armv5tl armv5tel armv5tejl armv6l armv6hl armv7l armv7hl armv7hnl armv8l armv8hl armv8hnl armv8hcnl

The macro doesn't contain the "aarch*" ARM architectures.

So I assume I should get rid of the '%{arm}' too, as it only covers architectures we don't offer. (leaving behind only the 'aarch64' and 's390x' strings)
Is that right?

Yes, that should be fine.

Alright, fixed in:
  https://src.fedoraproject.org/rpms/mysql8.0/c/06fc78c1d765173b183a968d1e2930936eae1f88?branch=rawhide

I also found one occurrence of 'ppc ppc64 ppc64p7', so I dropped it as well.

I think the only thing that's left is dealing with bundled boost in bug 2260138. Since that has a separate bug, I'm closing this as resolved.