Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Conversation

@toddbluhm
Copy link

Not sure if this applies to all linux distributions, but when trying to install this in a docker Alpine Linux distro I was having issues with isnan and isinf on Alpine Linux. The solution was to simply add the std namespace.

I know that adding the name space will break on Mac OS X so I added a check, and if on linux, then add the std namespace.

I would be great if someone could try this out on another version of linux and see if this breaks its.

@marcominetti
Copy link
Owner

I'll try it on Ubuntu and I'll merge if ok (in the weekend)... Thanks @toddbluhm

@toddbluhm
Copy link
Author

The first commit is bad. It broke on our CI server, but I think the second commit using cmath will work across the board. cmath apparently pulls math.h in and namespaces it to std, so thats really nice. The second commit worked on our CI server, Alpine, and Mac.

@marcominetti
Copy link
Owner

Can you squash the commits?

@toddbluhm
Copy link
Author

Done

@toddbluhm
Copy link
Author

Actually I take that back, its failing on our CI server still (Ubuntu). Not sure how it worked that one time...

@toddbluhm
Copy link
Author

Okay, so it looks like the reason my CI system was failing was due to that fact that we are using node v4 and our CI server defaults to gcc 4.6 and g++ 4.6 and the version of NAN used in this package requires a C++11 compiler (>= g++ 4.8). Once I updated gcc and g++ to 4.9 on our CI it installed just fine.

It worked that one time because I had not updated our CI to use node v4...it was still using node 0.10. As soon as I updated to node v4, it wanted the updated versions of gcc and g++.

@marcominetti
Copy link
Owner

ok, so can i close this pr without merging?

@toddbluhm
Copy link
Author

This PR is good and should be merged. It fixes the issue with using this lib in an Alpine linux distribution. The one caveat is that it requires having gcc/g++ >=4.8. But since this is also the same requirement that node v4 with node-gyp has, it should not be a problem for people who use this lib. If they need to support an older version, they can use the old node-memwatch lib.

marcominetti pushed a commit that referenced this pull request Nov 9, 2015
Use std::isnan and std::isinf when on linux.
@marcominetti marcominetti merged commit 3d90653 into marcominetti:master Nov 9, 2015
@marcominetti
Copy link
Owner

Ok, tested on Ubuntu, works fine.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants