Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests against all the mime-types; correct dependencies once and for all #713

Merged
merged 14 commits into from Jun 2, 2014

Conversation

@bf4
Copy link
Collaborator

@bf4 bf4 commented May 26, 2014

The most important parts of this PR are

  1. Loosening the dependency on mime-types from ~1.16 to >= 1.16 and < 3
  2. Adding appraisals to run tests across all these mime-types versions

There are a lot of PRs and issues related to this work, but none of them, I think fully (or correctly) addressed the issue.

@ConradIrwin I'm happy to add another commit to specify ruby 2.1 support.

Conditionals in a gemspec are only executed
when building the gem.

That is, when you run gem install, you get the built gem.
If the gem was built under CRuby 1.9.3, it would not specify
jruby-openssl or tlsmail as dependencies, no matter what
platform or version of Ruby you are running gem install on.

It only really makes sense to have them in the Gemfile for use in
development.
@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 26, 2014

Am battling Travis now.. hopefully I think I just found the bug in how it was overriding the BUNDLE_GEMFILE

@mcfiredrill
Copy link

@mcfiredrill mcfiredrill commented May 27, 2014

Nice, Appraisals is definitely the way to go here I think. 👍

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

@TravisCI @rkh looks like under 1.8.7 Travis isn't running export BUNDLE_GEMFILE=$PWD/gemfiles/mime_types_1.16.gemfile. Am I right that this is a Travis bug, not a config mistake? See https://travis-ci.org/mikel/mail/jobs/26091961

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

n/m re: 1.8.7, I found the problem was a typo in a gemfile name and a silent failure in travis

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

@ConradIrwin @mikel @jeremy @arunagw @jstirk @i'm only running rake (rake spec) for the tests. Should I also run rake corpus:verify_all? There's no clear instructions I've found on downloading the corpus and vendoring in corpus/spam.

And while I'm here, can we remove the references to tlsmail as 1.8.6 is no longer supported?

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

So, the failures for version 2.3 on ruby 1.9.3 and jruby look fixable

 1) Mail::Message helper methods == should ignore the message id value if other has a nil message id
     Failure/Error: m1.should eq m2

       expected: #<Mail::Message:36548840, Multipart: false, Headers: <Date: Tue, 27 May 2014 02:28:12 +0000>, <To: mikel@test.lindsaar.net>, <Subject: Yo!>, <Mime-Version: 1.0>, <Content-Type: text/plain>, <Content-Transfer-Encoding: 7bit>>
            got: #<Mail::Message:36535980, Multipart: false, Headers: <Date: Tue, 27 May 2014 02:28:11 +0000>, <To: mikel@test.lindsaar.net>, <Message-ID: 1234@test.lindsaar.net>, <Subject: Yo!>, <Mime-Version: 1.0>, <Content-Type: text/plain>, <Content-Transfer-Encoding: 7bit>>

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -#<Mail::Message:36548840, Multipart: false, Headers: <Date: Tue, 27 May 2014 02:28:12 +0000>, <To: mikel@test.lindsaar.net>, <Subject: Yo!>, <Mime-Version: 1.0>, <Content-Type: text/plain>, <Content-Transfer-Encoding: 7bit>>
       +#<Mail::Message:36535980, Multipart: false, Headers: <Date: Tue, 27 May 2014 02:28:11 +0000>, <To: mikel@test.lindsaar.net>, <Message-ID: 1234@test.lindsaar.net>, <Subject: Yo!>, <Mime-Version: 1.0>, <Content-Type: text/plain>, <Content-Transfer-Encoding: 7bit>>

     # ./spec/mail/message_spec.rb:1363
bf4 added 8 commits May 26, 2014
and then only when not running in Appraisal or
on TravisCI
The 'local_development' group is a way to specify gems
you may use while developing the gem, but aren't required
to run the tests.
The gemspec already specifies a version of mime-types.
Specifying it in the Gemfile is redundant and repetitive. :)
Run tests against all mime-types version with

    appraisal rake

or

    rake appraisal
@ioquatix
Copy link

@ioquatix ioquatix commented May 27, 2014

This looks really good! @bf4 please keep up the good work so we can get this sorted finally. Let me know if you require help.

@ConradIrwin @mikel @jeremy @arunagw @jstirk can we get some feedback on this?

@ConradIrwin
Copy link
Collaborator

@ConradIrwin ConradIrwin commented May 27, 2014

Thanks for looking into this.

I get issues running appraisal on my machine: https://gist.github.com/d2d2cecd9e6fa2aa0956. Not sure why it's trying to install ruby-debug on ruby 2.1.0?

Travis currently fails due to a problem in the edge build on ruby 1.9.3 — should that be reported upstream or moved to the "allowed failures" section?

Ruby 1.8.7 users will presumably need to manually lock their version of mime-types to 1.x? That shouldn't be a huge problem as most 1.8.7 code-bases I know of are not very active (though some of them are).

All in all this looks good though. We could probably reduce the matrix down to the latest versions of 1.x and 2.x, but that'd mostly be an optimization for travis' server cost.

@ioquatix
Copy link

@ioquatix ioquatix commented May 27, 2014

@bf4

And while I'm here, can we remove the references to tlsmail as 1.8.6 is no longer supported?

Seems like a good idea - 1.8.6 is ancient history now.

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

@ConradIrwin the failure on 1.9.3 is the same as the one I pasted above. It's really weird 'cause it seems to be impossible. Some race-condition thing? I'm not sure. I can't reproduce it. Check out the code in the test.

Did appraisal generate any lockfiles in the gemfiles dir? If not, try running just appraisal first.. it should bundle all those gems. If that works, then run appraisal rake. I considered adding that to the instructions but wasn't sure if it was just me as it wasn't in the appraisal docs.

You can 'manually' run and appraisal scenario with e.g. BUNDLE_GEMFILE=gemfiles/mime_types_edge.gemfile (bundle check || bundle) && rake

fwiw, I also created an rbx bug report, if anyone wants to help with that rubinius/rubinius#3050

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 27, 2014

@ConradIrwin I agree about reducing the matrix. I just wanted to be expansive for the purpose of this PR, especially since I've changed the dependency range to be so wide.

Would you mind trying to rerun the last build and see if we get different failures?

bf4 added 3 commits May 26, 2014
Exclude invalid ruby/mime-types combos from TravisCI
It looks like when @TravisCI doesn't find the
expected gemfile, it just skips bundling altogether,
but continues with no error message.

This leads to confusing failures.
@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 28, 2014

I see from the previous run that I forgot to exclude the 'latest' release of mime-types on Ruby 1.8.7. Rebased and force pushed, now with more docs in 'CONTRIBUTING' and a link to the rbx issue in .travis.yml

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 28, 2014

Yay, all passing!

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 29, 2014

Let's keep this revival momentum going! Merge and let's get a version pushed out! (Anyone know @mikel ?)

@mcfiredrill
Copy link

@mcfiredrill mcfiredrill commented May 29, 2014

Hope this gets merged 👍 👍

@ioquatix
Copy link

@ioquatix ioquatix commented May 29, 2014

@ConradIrwin @mikel @jeremy @arunagw @jstirk yes please merge so we can get this sorted!

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 29, 2014

@jstirk Awesome! did @mikel mention I emailed him about giving more ppl gem push and maybe commit rights? I want to help and get others to help, but I need y'all's help to do that :)

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 29, 2014

I kind of want to keep bumping this to keep momentum up. @halostatue any thoughts?

@halostatue
Copy link
Contributor

@halostatue halostatue commented May 29, 2014

Looks awesome to me. I think I may need to add a link to this discussion and the Travis page to the Contributions page in mime-types—we want to know that nothing added to mime-types breaks Mail before we release.

@ioquatix
Copy link

@ioquatix ioquatix commented May 29, 2014

@halostatue if people use Gemfile.lock correctly it shouldn't be an issue, but another thing you can do is test the code locally before you release i.e. pull the mail gem and run tests locally. It would be nice if Travis could test dependent projects automatically 👍 but currently I don't think it's possible.

@jstirk @bf4 🚋 looking forward to this release.

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented May 30, 2014

I'm starting to feel a little less optimistic about getting this merged and released. If not by the end of next week, I'll start a fork. Too many people rely on this code.

mikel added a commit that referenced this pull request Jun 2, 2014
Tests against all the mime-types; correct dependencies once and for all
@mikel mikel merged commit 2ef4180 into mikel:master Jun 2, 2014
1 check passed
1 check passed
@mikel
continuous-integration/travis-ci The Travis CI build passed
Details
@mikel
Copy link
Owner

@mikel mikel commented Jun 2, 2014

awesome work people.

@mcfiredrill
Copy link

@mcfiredrill mcfiredrill commented Jun 2, 2014

👍 awesome

@bf4
Copy link
Collaborator Author

@bf4 bf4 commented Jun 2, 2014

@ioquatix
Copy link

@ioquatix ioquatix commented Jun 2, 2014

@bf4 Thanks for sorting this out!!

bf4 added a commit to bf4/rails that referenced this pull request Jun 3, 2014
This allows Rails users to install mail 2.6 which relaxes
the mime-types dependency, which is a big win for a lot of people.

Previously, the mail gem restricted mime-types to ~> 1.16
but now it has expanded to [">= 1.16", "< 3"]

And the mime-types maintainer will also be checking that
2.x releases don't break mail.

See mikel/mail#713
https://rubygems.org/gems/mail/versions/2.6.0
@bf4 bf4 mentioned this pull request Jun 5, 2014
bf4 added a commit to bf4/rails that referenced this pull request Jun 8, 2014
This allows Rails users to install mail 2.6 which relaxes
the mime-types dependency, which is a big win for a lot of people.

Previously, the mail gem restricted mime-types to ~> 1.16
but now it has expanded to [">= 1.16", "< 3"]

And the mime-types maintainer will also be checking that
2.x releases don't break mail.

See mikel/mail#713
https://rubygems.org/gems/mail/versions/2.6.0

Related to rails#15493
soma added a commit to barsoom/rails that referenced this pull request Jan 15, 2015
This allows Rails users to install mail 2.6 which relaxes
the mime-types dependency, which is a big win for a lot of people.

Previously, the mail gem restricted mime-types to ~> 1.16
but now it has expanded to [">= 1.16", "< 3"]

And the mime-types maintainer will also be checking that
2.x releases don't break mail.

See mikel/mail#713
https://rubygems.org/gems/mail/versions/2.6.0

Conflicts:
	actionmailer/actionmailer.gemspec
DanielVartanov added a commit to veeqo/rails that referenced this pull request Aug 16, 2016
This allows Rails users to install mail 2.6 which relaxes
the mime-types dependency, which is a big win for a lot of people.

Previously, the mail gem restricted mime-types to ~> 1.16
but now it has expanded to [">= 1.16", "< 3"]

And the mime-types maintainer will also be checking that
2.x releases don't break mail.

See mikel/mail#713
https://rubygems.org/gems/mail/versions/2.6.0
infertux added a commit to infertux/rails that referenced this pull request Feb 10, 2017
This allows Rails users to install mail 2.6 which relaxes
the mime-types dependency, which is a big win for a lot of people.

Previously, the mail gem restricted mime-types to ~> 1.16
but now it has expanded to [">= 1.16", "< 3"]

And the mime-types maintainer will also be checking that
2.x releases don't break mail.

See mikel/mail#713
https://rubygems.org/gems/mail/versions/2.6.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants