Stub mini-mime in for mime-types #14525

NickLaMuro · 2017-03-27T17:01:40Z

Uses a stub for the two mime-types methods used by rest-client and mail (the only dependencies of mime-types) and forwards them on to mini-mime. Should reduce the memory footprint from that gem... hopefully...

Before

After

For those stopping in (👋) , the links to the rest-client PR will provide some context to why this PR exists.

Links

Original "offending" (and unmerged) PR on rest-client: PERF: Eliminate dependency on mime-types gem rest-client/rest-client#557
- rebased here (still open): PERF: Eliminate dependency on mime-types gem rest-client/rest-client#644

NickLaMuro · 2017-03-27T17:04:45Z

For the record, have not ran anything to confirm that this works as an interface, only that the application loads when this is in place. Further tests will need to be done before we even consider merging this.

Fryguy · 2017-03-27T17:05:53Z

mime-types-redirector/lib/mime-types.rb

+    end
+
+    def self.type_for filename
+      Array.wrap MiniMime.lookup_by_filename(filename)


I think you need a .collect(&:content_type) here?

Array.wrap(MiniMime.lookup_by_filename(filename).content_type)

@Fryguy Well, I was going off of this change:

https://github.com/rest-client/rest-client/pull/557/files#diff-3724e1adc91ded3d8e979d73f91f1b1eL915

So I don't think so, but maybe I read that diff wrong (it was Monday morning at the time of writing this code...)

Oh... I think I see the error in my ways now...

I think I was right the first time... see #14525 (comment)

Fryguy · 2017-03-27T17:39:32Z

@NickLaMuro Also see this one: https://github.com/mikel/mail/pull/1060/files#diff-636a445469a465e4b5088a2b37b105b1L102

Fryguy · 2017-03-27T17:41:19Z

Once this gets nailed down, this might be a good addition to the mini_mime gem. This way they have a workaround to intercepting mime/types in places where rest-client or mail can't be upgraded.

NickLaMuro · 2017-03-27T17:46:26Z

Once this gets nailed down, this might be a good addition to the mini_mime gem. This way they have a workaround to intercepting mime/types in places where rest-client or mail can't be upgraded.

I agree, though I am not sure how we could do that from within the mini-mime gem itself. This works because we tricking bundler in that this is the dependency for mime-types, when really it is just a stub. I don't think we could do that with mini-mime alone, and the contribution would more be a README "howto" entry I think.

NickLaMuro · 2017-03-27T17:52:06Z

@NickLaMuro Also see this one: https://github.com/mikel/mail/pull/1060/files#diff-636a445469a465e4b5088a2b37b105b1L102

@Fryguy I think the way that is called, that is different, but I did see that one, which is why I was confused for a bit. But looking at this:

https://github.com/ManageIQ/manageiq/blob/master/app/models/filesystem.rb#L111-L118

Looks like I think returning a MIME::Type is what is expected, and a content_type should be fetched from that (FYI, .of is just an alias for .type_for).

blomquisg · 2017-05-04T19:37:17Z

Booted myself from the reviewers list. Mime types are just not anything I feel I can give an up or down vote.

miq-bot · 2017-05-26T14:41:57Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-06-14T19:12:02Z

@NickLaMuro Do you have any numbers for the memory/time savings? I thought they were in this thread, but i can't find them.

Fryguy · 2017-06-14T19:13:36Z

mime-types-redirector/mime-types.gemspec

+  s.summary = "Stub mime-types repo that redirects to mini-mime"
+
+  s.add_dependency 'mini_mime', '>= 0.1.1'
+end


I'm wondering if we can just make this a released gem somehow, but I can't think of how you would make the Gemfile work. 🤔

Yeah, that was my thing too. We can't make this quack as a dependency that is resolvable using rubygems, and so the best we could do is move this into a git repo... but that even seems overkill.

Not completely opposed to the idea, though, hate adding another git gem to the repo... Either way, I would probably put some tests around it if we wanted to do that.

NickLaMuro · 2017-06-14T19:13:49Z

@Fryguy Yeah, I feel like we might have talked about this in chat or something with the people involved, but it is around a 10MiB savings.

Regardless, will do a quick test before and after and add it to the top.

NickLaMuro · 2017-06-14T19:35:12Z

@Fryguy well, I have been testing for probably the past 20 min, and the results are varied enough where I don't think putting them here in full will help.

But the short version is what I have here is basically a 0.0MiB for the require "mime-types", and master is around 3MiB and up (depends on the mood I guess...).

Fryguy · 2017-06-14T21:29:40Z

@NickLaMuro Can you verify that with the change, that the UI classic repo still passes? I am concerned because UI classic still references capybara which, in turn, still uses the mime-types gem and I'm concerned that it will try to load the gem anyway, clobbering your changes.

Uses a stub for the two mime-types methods used by rest-client and mail (the only dependencies of mime-types) and forwards them on to mini-mime. Should reduce the memory footprint from that gem... hopefully...

NickLaMuro · 2017-06-20T21:26:40Z

Said in a PM to @Fryguy, but I used this branch to run the test suite for manageiq-ui-classic, and everything passed in both suites except one test. That test failed only the first time (out of two), and it was a discrepancy in expected json keys, which doesn't point to an issue with this mime-types change.

miq-bot · 2017-06-20T21:42:57Z

Checked commit NickLaMuro@8cfab3b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 2 offenses detected

mime-types-redirector/lib/mime-types.rb

❗ - Line 13, Col 7 - Style/Alias - Use alias instead of alias_method in a class body.
❗ - Line 1, Col 1 - Style/FileName - The name of this source file (mime-types.rb) should use snake_case.

Fryguy · 2017-06-23T23:05:37Z

Looks good to me...merging, and then let's see how it plays out.

Fryguy · 2017-06-23T23:07:58Z

Fryguy · 2017-06-26T15:50:58Z

@NickLaMuro I've opened discourse/mini_mime#6 to see if we can get this into mini_mime in some form, if they are willing to accept it.

NickLaMuro · 2017-06-26T15:55:13Z

@Fryguy Thanks! I think having a more official solution is probably for the best.

himdel · 2017-06-29T14:28:40Z

Note this also seems to break running rspec foo.. in ui-classic in some cases.. (because of that bundler bug it would seem)...

The path /home/hkataria/dev/manageiq-ui-classic/mime-types-redirector does not
exist.

But making a symlink to that dir fixes that :)

NickLaMuro · 2017-06-29T14:38:54Z

@himdel Do you have a link to what test was failing with this? I might see if there is another bug with bundler that I need to look into with this.

NickLaMuro · 2017-06-29T14:48:50Z

Note this also seems to break running rspec foo.. in ui-classic in some cases.. (because of that bundler bug it would seem)...

As just a minor FYI, this only fixes the problem where bundle install followed by a bundle install --deployment doesn't work because of incorrect relative path resolution from a given root (basically, using the wrong root). I think there might be a different bug in which the root for the path is resolved incorrectly, and I might need to look into that.

himdel · 2017-06-30T09:02:50Z

@NickLaMuro It was not a specific test failing, it was not being able to run any specs via rspec. But.. it is possible it would have worked with bundle exec. Right now if I run it without, it actually complains that The path '/home/himdel/manageiq-ui-classic/spec/manageiq-ui-classic' does not exist. for some reason :).

EDIT:

And @martinpovolny just hit the manageiq-ui-classic/mime-types-redirector does not exist bug even when running bundle exec rspec ... in ui-classic.

Relative pathing can have problems if the pwd is not the manageiq directory. This ensures the correct path. Related: - ManageIQ#14525 - ManageIQ/manageiq-ui-classic#1606 - ManageIQ/manageiq-content#136

NickLaMuro changed the title ~~Stub mini-mime in for mime-types~~ [WIP] Stub mini-mime in for mime-types Mar 27, 2017

Fryguy reviewed Mar 27, 2017

View reviewed changes

miq-bot added dependencies wip labels Mar 27, 2017

NickLaMuro force-pushed the mini-mime-mime-types-stub branch from 40f284d to 09dffb6 Compare March 27, 2017 17:31

chessbyte assigned Fryguy Mar 29, 2017

chessbyte added the core label Mar 29, 2017

chessbyte requested review from blomquisg and gtanzillo March 29, 2017 13:00

blomquisg removed their request for review May 4, 2017 19:35

miq-bot added the unmergeable label May 26, 2017

NickLaMuro force-pushed the mini-mime-mime-types-stub branch from 09dffb6 to f2fd8c5 Compare June 6, 2017 16:20

NickLaMuro changed the title ~~[WIP] Stub mini-mime in for mime-types~~ Stub mini-mime in for mime-types Jun 6, 2017

miq-bot removed unmergeable wip labels Jun 6, 2017

Fryguy reviewed Jun 14, 2017

View reviewed changes

NickLaMuro force-pushed the mini-mime-mime-types-stub branch from f2fd8c5 to 3879297 Compare June 14, 2017 19:41

Stub mini-mime in for mime-types

8cfab3b

Uses a stub for the two mime-types methods used by rest-client and mail (the only dependencies of mime-types) and forwards them on to mini-mime. Should reduce the memory footprint from that gem... hopefully...

NickLaMuro force-pushed the mini-mime-mime-types-stub branch from 3879297 to 8cfab3b Compare June 20, 2017 21:37

Fryguy merged commit bb244dc into ManageIQ:master Jun 23, 2017

Fryguy added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 23, 2017

himdel mentioned this pull request Jun 26, 2017

travis - run bundler with --no-deployment ManageIQ/manageiq-ui-classic#1606

Merged

jhernand mentioned this pull request Jul 3, 2017

Use 'application/xml' explicitly ManageIQ/ovirt#83

Merged

Fryguy mentioned this pull request Jul 3, 2017

Use full path in the Gemfile #15503

Merged

NickLaMuro mentioned this pull request Jul 26, 2017

[RFE] PRs should support add_reviewer action ManageIQ/miq_bot#337

Closed

This was referenced Dec 1, 2017

Allow rest-client to work again in automate methods ManageIQ/manageiq-automation_engine#125

Closed

Move mime-types-redirector to fake gemdir #16581

Closed

NickLaMuro mentioned this pull request May 30, 2018

Update dependencies to match new fog-google #17258

Merged

NickLaMuro mentioned this pull request Oct 17, 2018

DB Backups to Openstack Swift ManageIQ/manageiq-gems-pending#371

Merged

simaishi mentioned this pull request Nov 9, 2020

Allow bundler to do the install with the correct build options ManageIQ/manageiq-rpm_build#116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stub mini-mime in for mime-types #14525

Stub mini-mime in for mime-types #14525

NickLaMuro commented Mar 27, 2017 •

edited

Loading

NickLaMuro commented Mar 27, 2017

Fryguy Mar 27, 2017 •

edited

Loading

NickLaMuro Mar 27, 2017

NickLaMuro Mar 27, 2017

NickLaMuro Mar 27, 2017

Fryguy commented Mar 27, 2017

Fryguy commented Mar 27, 2017

NickLaMuro commented Mar 27, 2017

NickLaMuro commented Mar 27, 2017 •

edited

Loading

blomquisg commented May 4, 2017

miq-bot commented May 26, 2017

Fryguy commented Jun 14, 2017

Fryguy Jun 14, 2017

NickLaMuro Jun 14, 2017

NickLaMuro commented Jun 14, 2017

NickLaMuro commented Jun 14, 2017 •

edited

Loading

Fryguy commented Jun 14, 2017 •

edited

Loading

NickLaMuro commented Jun 20, 2017

miq-bot commented Jun 20, 2017

Fryguy commented Jun 23, 2017

Fryguy commented Jun 23, 2017

Fryguy commented Jun 26, 2017

NickLaMuro commented Jun 26, 2017

himdel commented Jun 29, 2017 •

edited

Loading

NickLaMuro commented Jun 29, 2017

NickLaMuro commented Jun 29, 2017

himdel commented Jun 30, 2017 •

edited

Loading

Stub mini-mime in for mime-types #14525

Stub mini-mime in for mime-types #14525

Conversation

NickLaMuro commented Mar 27, 2017 • edited Loading

Links

NickLaMuro commented Mar 27, 2017

Fryguy Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

NickLaMuro Mar 27, 2017

Choose a reason for hiding this comment

NickLaMuro Mar 27, 2017

Choose a reason for hiding this comment

NickLaMuro Mar 27, 2017

Choose a reason for hiding this comment

Fryguy commented Mar 27, 2017

Fryguy commented Mar 27, 2017

NickLaMuro commented Mar 27, 2017

NickLaMuro commented Mar 27, 2017 • edited Loading

blomquisg commented May 4, 2017

miq-bot commented May 26, 2017

Fryguy commented Jun 14, 2017

Fryguy Jun 14, 2017

Choose a reason for hiding this comment

NickLaMuro Jun 14, 2017

Choose a reason for hiding this comment

NickLaMuro commented Jun 14, 2017

NickLaMuro commented Jun 14, 2017 • edited Loading

Fryguy commented Jun 14, 2017 • edited Loading

NickLaMuro commented Jun 20, 2017

miq-bot commented Jun 20, 2017

Fryguy commented Jun 23, 2017

Fryguy commented Jun 23, 2017

Fryguy commented Jun 26, 2017

NickLaMuro commented Jun 26, 2017

himdel commented Jun 29, 2017 • edited Loading

NickLaMuro commented Jun 29, 2017

NickLaMuro commented Jun 29, 2017

himdel commented Jun 30, 2017 • edited Loading

NickLaMuro commented Mar 27, 2017 •

edited

Loading

Fryguy Mar 27, 2017 •

edited

Loading

NickLaMuro commented Mar 27, 2017 •

edited

Loading

NickLaMuro commented Jun 14, 2017 •

edited

Loading

Fryguy commented Jun 14, 2017 •

edited

Loading

himdel commented Jun 29, 2017 •

edited

Loading

himdel commented Jun 30, 2017 •

edited

Loading