An example of how mathn usage could be made explicit and optional #48

Closed
wants to merge 27 commits into
from

Projects

None yet

7 participants

@jasonhutchens

See my comment on issue #43 for information about this change.

The implicit inclusion of mathn breaks another gem we use. The bug is with mathn itself, but we find ourselves in a position where we need to use ruby-units and the other gem together.

@jasonhutchens

Don't implicitly require mathn

@jasonhutchens

Include mathn explicitly in the tests if the WITHOUT_MATHN environment variable is not set.

Run bundle exec rspec for normal tests and WITHOUT_MATHN=1 bundle exec rspec for tests without mathn.

@jasonhutchens

Requiring mathn also brings in Complex. This handles the case where the user explicitly requires mathn or even just Complex by itself.

Most of the other changes are to do with the fact that Complex may not be present in the namespace.

@olbrich
Owner

Thanks, I'll take a look. I'm not too fond of the idea of not including mathn, but this seems like a reasonable compromise.

@jasonhutchens

No problem, if you decide not to include it we'll just continue to use our fork of ruby-units with mathn removed.

@matadon

Requiring mathn also breaks SimpleCov[1] right now; it's probably a VM or standard library bug, but it's still a pretty nasty issue.

[1] colszowka/simplecov#140

@olbrich
Owner

Interesting. I'll take another look at this. I've been considering if it might be possible to get the desired behavior by including a different entry point. So you could do things like:

gem 'ruby-unit', :include => 'ruby-units/no_mathn'

@jasonhutchens

Turns out that using ruby-units also breaks the DateHelper in Rails: rails/rails#656

@matpowel matpowel referenced this pull request in jruby/jruby Mar 9, 2013
Closed

Time.at broken in 1.7.3 when passed (Time, 0) #565

jasonhutchens and others added some commits Mar 27, 2013
@jasonhutchens jasonhutchens fix the redefinition of Time to allow Time.at(Time), which fixes Timecop bcef4ae
@jasonhutchens jasonhutchens Version bump to 1.4.3.3 057070c
@jasonhutchens jasonhutchens regenerate the gemspec after version bumpage d03078c
@stwr667 stwr667 Fixing unit parse for powered numbers per 100<x>
WEB-9116 #time 4h
This is the "Stack level too deep" problem we were having. It's
quite a rare case for it to happen. It happens only when the
numerator of a unit is a power (e.g. kg^2) and the denominator
starts with a 100<x> (e.g. 100l). Due to string-matching, it
incorrectly assumes that this unit is a rational number.
When doing this, it never allows the Unit's constructor to fully
complete because instead of being able to instantiate a
scalarless (i.e. scalar=1) unit, it continually creates scalar
values of e.g. 2/100, which equals 1/50. Hence it never settles.

This fixes it.
ed98f0d
@stwr667 stwr667 Merge pull request #1 from agworld/hotfix/WEB-9116
Fixing unit parse for powered numbers per 100<x>
9150116
@stwr667 stwr667 Just ran `be rake gemspec` to update required versions on website fb1e6e9
@matpowel matpowel Merge remote-tracking branch 'upstream/master' e3c8d46
@matpowel matpowel fix large bug when mathn is not required because of Fixnum division 3d7445f
@matpowel matpowel bump gemspec version properly 58ab3b5
@stwr667

👍

@EntilZha

I ran into this issue and chased it to this pull request. Would love to see it merged in, but if not, will this fix my problem (simplecov seg fault because of mathn)

gem 'ruby-unit', :include => 'ruby-units/no_mathn'

if not, could you link me to the fork you speak of @jasonhutchens ?

@EntilZha

For some reason I am getting this error when including it:
Could not find gem 'ruby-unit (>= 0) ruby' in
git://github.com/agworld/ruby-units.git (at master).
Source does not contain any versions of 'ruby-unit (>= 0) ruby'

via this: gem 'ruby-unit', :git => 'git://github.com/agworld/ruby-units.git'

Maybe I'me missing something very simple...

@jasonhutchens

@EntilZha try this:

gem "ruby-units", :git => 'git@github.com:agworld/ruby-units.git'
@EntilZha

Same problem. I tried it a few other ways, gem install, bundle install, etc, all have the same error. Giving a try to cloning the source and installing it from that, although the workflow is a lot better to get it working with the Gemfile

@olbrich
Owner

FYI, I'll be taking a look at this fix next.

@jasonhutchens

Sorry @EntilZha, I'm not sure what's causing your problem. Also @olbrich, if it makes it easier, I can prepare a new branch with just the changes we made specific to this issue? Looks like there are lots of unrelated commits above.

@EntilZha
@dkubb

FWIW mathn also currently breaks active_shipping and spree_active_shipping.

@olbrich
Owner

@dkubb Have you reported the mathn problems to the appropriate projects? It seems to me that if they are broken by inclusion of something from the standard library, then that's a bug.

@jasonhutchens

That's what I said a year ago @olbrich: "The implicit inclusion of mathn breaks another gem we use. The bug is with mathn itself, but we find ourselves in a position where we need to use ruby-units and the other gem together."

@olbrich
Owner

Ok, I'm going to try a new approach here to fixing this issue. This particular pull request is old enough and has enough other cruft in it that it's difficult to keep track of the relevant changes. I've made a new branch called 'without-mathn', if you want to help me fix it, please fork and work on that branch.

In general I don't like the idea of having configuration determine the behavior of the gem itself. I'd prefer for people who need this functionality to just be able to specify the branch in their Gemfile. Of course this means maintaining two parallel branches for development going forward.

I do consider the fact that other gems break when mathn is required to be a bug in those gems, and not with ruby-units.

I think the actual fix for this is to contribute fixes to the affected gems so that requiring mathn doesn't break them, but I'm willing to explore methods for reducing ruby-unit's dependency on 'mathn'.

@dkubb

@olbrich yeah, I'm working on patches for the affected projects.

@jasonhutchens

"I do consider the fact that other gems break when mathn is required to be a bug in those gems, and not with ruby-units" is fair enough and quite proper, but completely doesn't work when people are trying to use ruby-units in real-world projects. And bear in mind that the maintainers of other projects would probably disagree with you, as the bug is a side-effect of using mathn. In our case, it was a gem calling nan? on the results of Math.sqrt(n) that only manifests itself when the result of that call is an integer. So the real bug is in mathn, a standard library included with the Ruby distribution. We should get that fixed. But, again, getting a bug fixed in a Ruby standard library should not be a prerequisite to us using ruby-units.

"In general I don't like the idea of having configuration determine the behavior of the gem itself" is a misunderstanding of the solution I proposed. I suggested that ruby-units should take advantage of mathn only if the user requires it explicitly. No configuration required.

"This particular pull request is old enough and has enough other cruft in it...". Note that the other cruft is:

  • subtract not returning a Unit type for 0 quantities
  • allow Unit conversion of aliased temperatures
  • fix redefinition of Time to allow TimeCop to work (Time.at goes missing)
  • fix unit parsing to avoid a stack exception with some unit combinations
  • more tests

Please also note that using ruby-units currently breaks:

  • TimeCop
  • RGeo
  • Alchemist
  • ActiveShipping
  • pry-rails

Anyway, we'll continue using our fork; we really have no choice!

@olbrich
Owner

FYI,

Simplecov seems to work fine if you are using a recent version. I think they fixed their problem with mathn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment