Bug 1859414 - Review Request: rubygem-actionmailbox - Email composition and delivery framework (part of Rails)
Summary: Review Request: rubygem-actionmailbox - Email composition and delivery framew...
Keywords:
Status: MODIFIED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-07-22 00:25 UTC by Pavel Valena
Modified: 2020-08-11 14:56 UTC (History)
2 users (show)

Fixed In Version: rubygem-actionmailbox-6.0.3.1-1.fc33
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
vondruch: fedora-review+


Attachments (Terms of Use)

Description Pavel Valena 2020-07-22 00:25:43 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01571675-rubygem-actionmailbox/rubygem-actionmailbox.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/pvalena/ruby-on-rails/fedora-rawhide-x86_64/01571675-rubygem-actionmailbox/rubygem-actionmailbox-6.0.3.1-1.fc33.src.rpm
Description: Email on Rails. Compose, deliver, and test emails using the familiar controller/view pattern. First-class support for multipart email and attachments.
Fedora Account System Username: pvalena

COPR build: https://copr.fedorainfracloud.org/coprs/build/1571675

This is part of Upgrade to Ruby on Rails 6.0: https://fedoraproject.org/wiki/Changes/Ruby_on_Rails_6.0

Comment 2 Vít Ondruch 2020-08-07 14:01:20 UTC
Let me take a look.

Comment 3 Vít Ondruch 2020-08-07 14:07:05 UTC
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

Comment 4 Vít Ondruch 2020-08-07 14:16:24 UTC
* 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

Comment 5 Vít Ondruch 2020-08-07 14:17:31 UTC
Sorry, I accidentally send the review prematurely :/

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

Comment 6 Vít Ondruch 2020-08-07 15:22:12 UTC
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 ...
~~~

Comment 7 Pavel Valena 2020-08-07 17:03:20 UTC
(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.

Comment 9 Vít Ondruch 2020-08-10 11:07:10 UTC
(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.

Comment 10 Pavel Valena 2020-08-11 01:22:18 UTC
> 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!

Comment 11 Vít Ondruch 2020-08-11 07:49:04 UTC
(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

Comment 12 Gwyn Ciesla 2020-08-11 12:54:47 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-actionmailbox


Note You need to log in before you can comment on or make changes to this bug.