Bug 1859414

I've cleaned up SPEC file: https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01588154-rubygem-actionmailbox/rubygem-actionmailbox.spec

SRPM: https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01588154-rubygem-actionmailbox/rubygem-actionmailbox-6.0.3.1-1.fc33.src.rpm
COPR build: https://copr.fedorainfracloud.org/coprs/build/1588154

Let me take a look.

Pavel Valena from comment #1)
> SRPM:
> https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/
> fedora-rawhide-x86_64/01588154-rubygem-actionmailbox/rubygem-actionmailbox-6.
> 0.3.1-1.fc33.src.rpm


The URL ^^ does not work. I think I am supposed to use the following URLs now, right?

https://copr-be.cloud.fedoraproject.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01591362-rubygem-actionmailbox/rubygem-actionmailbox.spec
https://copr-be.cloud.fedoraproject.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01591362-rubygem-actionmailbox/rubygem-actionmailbox-6.0.3.1-1.fc33.src.rpm(In reply to Pavel

* Never release available upstream
  - actionmailbox 6.0.3.2 is available upstream already. But I guess you want to keep this in sync with the rest of the RoR, so no big deal ATM.

* Summary and description
  - The summary and description differs from the upstream versions, is that intentional?

* tests and tools in separate archives
  - What is the reason? Wouldn't it be enough to keep them in one archive?
  - Also, while minor nit, there is probably missing '/' in the command for tests.

* Missing space after `Requires:`
  - There is missing space in `-doc` subpackage:

~~~
Requires:%{name} = %{version}-%{release}
~~~

* Keep the default generated BRs together

Sorry, I accidentally send the review prematurely :/

* Keep the default generated BRs together:
  - It would be nice to keep these together:

And again. What is going on with my browser or !@!@#!@#@!!@#

* Keep the default generated BRs together:
  - It would be nice to keep these together:

~~~
BuildRequires: ruby(release)
BuildRequires: rubygems-devel
BuildRequires: ruby >= 2.5.0
~~~

  - Mixing rubygems-devel in between other requires for instance does not make the review/maintenance easier.

* Circular dependency with rubygem-rails
  - I think that using `rails` gem on various places is shortcut which brings its own issues. I think it would be better to list all the dependencies excluding `rails` itself.

* Keep the original tests in place
  - I prefer to keep the expanded tests in place and copy them to the location they needs to be. But it might be confusing either way.

* Simpler test execution
  - There is too much boilerplate in the `%check` section. I think this would be enough:

~~~
... snip ...

# Let's keep Requires and BuildRequires sorted alphabeticaly
BuildRequires: rubygem(actionmailer) = %{version}
BuildRequires: rubygem(activestorage) = %{version}
BuildRequires: rubygem(bundler)
BuildRequires: rubygem(railties) = %{version}
BuildRequires: rubygem(sprockets-rails)
BuildRequires: rubygem(sqlite3)
BuildRequires: rubygem(webmock)

... snip ...

%check
pushd .%{gem_instdir}
ln -s %{_builddir}/tools ..
cp -a %{_builddir}/test .

export BUNDLE_GEMFILE=${PWD}/Gemfile

# At least one dependency less.
sed -i '/byebug/ s/^/#/' test/test_helper.rb

cat > $BUNDLE_GEMFILE <<EOF
gem "railties"
gem "actionmailer"
gem "activestorage"
gem "sprockets-rails"
gem "sqlite3"
gem "webmock"
EOF

ruby -rbundler -Itest -e 'Dir.glob "./test/**/*_test.rb", &method(:require)'
popd

... snip ...
~~~

(In reply to Vít Ondruch from comment #3)
> The URL ^^ does not work. I think I am supposed to use the following URLs
> now, right?
> 
> https://copr-be.cloud.fedoraproject.org/results/pvalena/ruby-on-rails/fedora-
> rawhide-x86_64/01591362-rubygem-actionmailbox/rubygem-actionmailbox.spec
> https://copr-be.cloud.fedoraproject.org/results/pvalena/ruby-on-rails/fedora-
> rawhide-x86_64/01591362-rubygem-actionmailbox/rubygem-actionmailbox-6.0.3.1-
> 1.fc33.src.rpm(In reply to Pavel

Yes, sorry I forgot it'll get cleaned up on rebuild. 1591362 is a correct (latest) build.
FTR, this is git-based link to spec file: https://copr-dist-git.fedorainfracloud.org/cgit/pvalena/ruby-on-rails/rubygem-actionmailbox.git/plain/rubygem-actionmailbox.spec?id=9e33f0b7121acd9236d874f1a50adebd40eb41c8

> * Never release available upstream
>   - actionmailbox 6.0.3.2 is available upstream already. But I guess you want to keep this in sync with the rest of the RoR, so no big deal ATM.

Yes, I have most already built in side-tag: https://koji.fedoraproject.org/koji/builds?userID=&tagID=26290&order=-build_id&inherited=0&latest=1
I will run it through "update" automation (build in side-tag once more), when 6.0.3.1 is merged in master. 

>  * Summary and description
>  - The summary and description differs from the upstream versions, is that intentional?

I will fix that. Most probably it was changed when bumping to 6.0.3 (I didn't check those differences much).

>  * tests and tools in separate archives
>   - What is the reason? Wouldn't it be enough to keep them in one archive?

rails-*-tools.txz is one archive with tools shared accross most of RoR, as it's needed for running most of the test suites. I've decided to archive/manage it separately.

>   - Also, while minor nit, there is probably missing '/' in the command for tests.

Doesn't have to be there, and AFAIK it doesn't change the behaviour. I'll add it for readability.

>  * Missing space after `Requires:`
>   - There is missing space in `-doc` subpackage:

Fixed.

>  * Keep the default generated BRs together:
>   - It would be nice to keep these together:
>   - Mixing rubygems-devel in between other requires for instance does not make the review/maintenance easier.

Right, the intent was to have them sorted alphabetically, as the comment says (I've used `sort -u`).
But sure, I will revert that. I have no preference (as the whole `BuildRequires:` concept is wrong IMO).

>  * Circular dependency with rubygem-rails
>   - I think that using `rails` gem on various places is shortcut which brings its own issues. I think it would be better to list all the dependencies excluding `rails` itself.

Sure, let's trim those down. (Note that it's only a runtime dependency, so it does't matter until something depends on actionmailbox at buildtime.)

>  * Keep the original tests in place
>   - I prefer to keep the expanded tests in place and copy them to the location they needs to be. But it might be confusing either way.

Ok. Is there any benefit to having them duplicated?

>  * Simpler test execution
>   - There is too much boilerplate in the `%check` section. I think this would be enough:

Sure, let's do that instead. I somehow preferred the current (generic) approach, but it makes sense to trim it down.


Thanks for the review!
_ _ _ _

The changed spec will will follow in a separate comment.

SPEC: https://copr-dist-git.fedorainfracloud.org/cgit/pvalena/ruby-on-rails/rubygem-actionmailbox.git/plain/rubygem-actionmailbox.spec?id=b361974ce43543dae42f11389e6afbe3377e1cbb 
SRPM: https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01596166-rubygem-actionmailbox/rubygem-actionmailbox-6.0.3.1-1.fc33.src.rpm
COPR BUILD: https://copr.fedorainfracloud.org/coprs/pvalena/ruby-on-rails/build/1596166/
DIFF: https://copr-dist-git.fedorainfracloud.org/cgit/pvalena/ruby-on-rails/rubygem-actionmailbox.git/commit/?id=b361974ce43543dae42f11389e6afbe3377e1cbb

(In reply to Pavel Valena from comment #7)
> (In reply to Vít Ondruch from comment #3)
> >   - Also, while minor nit, there is probably missing '/' in the command for tests.
> 
> Doesn't have to be there, and AFAIK it doesn't change the behaviour. I'll
> add it for readability.

I don't mind if you remove the `\` after tools, but I mind consistency. Thx

> >  * Keep the default generated BRs together:
> >   - It would be nice to keep these together:
> >   - Mixing rubygems-devel in between other requires for instance does not make the review/maintenance easier.
> 
> Right, the intent was to have them sorted alphabetically, as the comment
> says (I've used `sort -u`).

Strange, how the `rubygems-devel` got mixed in between `rubygems()` then? Does the sort ignore brackets?

Still, I would prefer to keep the default BRs on top, but that is my preference, I'll leave it up to you.

> >  * Circular dependency with rubygem-rails
> >   - I think that using `rails` gem on various places is shortcut which brings its own issues. I think it would be better to list all the dependencies excluding `rails` itself.
> 
> Sure, let's trim those down. (Note that it's only a runtime dependency, so
> it does't matter until something depends on actionmailbox at buildtime.)

Not sure I understand what you mean by this? rubygem-rails depends on rubygem-actionmailbox and rubygem-actionmailbox had `BR: rubygem-rails`. That is IMO circular dependency which is good to break.

So now it is much better.

> >  * Keep the original tests in place
> >   - I prefer to keep the expanded tests in place and copy them to the location they needs to be. But it might be confusing either way.
> 
> Ok. Is there any benefit to having them duplicated?

I am not saying there is benefit in duplication, but shuffling the directory on the disk does not help either. Of course I would prefer symlink if that was possible, but otherwise I think the `cp` has the closest behavior to `mv`. May be it could help if linked just the files, but that is OT for this review (unless you want to experiment a bit ;) ) ...

> Thanks for the review!

YAW. The package looks good => APPROVED.

> Strange, how the `rubygems-devel` got mixed in between `rubygems()` then? Does the sort ignore brackets?

Yes, it seems strange to me as well. I should have probably used `sort -h`

> Still, I would prefer to keep the default BRs on top, but that is my preference, I'll leave it up to you.

Right, I don't mind either way, so I've moved them to the top. Changes compared to gem2rpm-generated file: https://gist.github.com/032d0ca905e984a9f92b9b56370dd8a8

> Not sure I understand what you mean by this? rubygem-rails depends on rubygem-actionmailbox

Yes, but the dependency is runtime-only (no BR). But you're right, it's better.

> I am not saying there is benefit in duplication, but shuffling the directory on the disk does not help either. Of course I would prefer symlink if that was possible, but otherwise I think the `cp` has the closest behavior to `mv`. May be it could help if linked just the files, but that is OT for this review (unless you want to experiment a bit ;) ) ...

We could hardlnink / bind / mount, but that could potentialy complicate or destabilze the test suite; and I don't want add bash code just for doing symlinks. Let's leave that for upstream to solve first.

Thanks for the review!

(In reply to Pavel Valena from comment #10)
> > Not sure I understand what you mean by this? rubygem-rails depends on rubygem-actionmailbox
> 
> Yes, but the dependency is runtime-only (no BR). But you're right, it's
> better.

Yes, rails has runtime dependency on rubygem-actionmailbox, but that means if you install rubygem-rails for rubygem-actionmailbox build, also rubygem-actionmailbox must be installed, but you don't have if for rubygem-actionmailbox build, right? That would need bootstrap round.


> Thanks for the review!

YAW

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-actionmailbox