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

Define fmin() for Visual Studio #1151

Merged
merged 3 commits into from Nov 20, 2016

Conversation

Projects
None yet
5 participants
@nilgoyette
Contributor

nilgoyette commented Nov 18, 2016

Should fix issue #1138.

As @matthew-brett wrote, fmin() is not defined in the "old" versions of Visual Studio.
I defined it in dpy_math.h, used it in vox2track.pyx and ran setup.py bdist_wheel on Windows and Linux Mint to test the broken builds. Seem good.

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

Thanks! I will launch a try_branch on one of the bots.

@arokem

This comment has been minimized.

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

Harumph. It seems to be getting master instead of this branch. I'll keep trying

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

I seem to be failing at this. I am quite confident that I am doing the same thing that I always do. Checkout the branch and then run:

try_branch.py dipy-bdist32-27

But it seems to still be getting the most recent commit on master. For example: http://nipy.bic.berkeley.edu/builders/dipy-bdist32-27/builds/387. I do notice that the relevant patch is somehow in there as well: http://nipy.bic.berkeley.edu/builders/dipy-bdist32-27/builds/387/steps/git/logs/patch, but it doesn't get used, and the hash points to current master. @matthew-brett : any ideas about what might be going on?

@coveralls

This comment has been minimized.

coveralls commented Nov 18, 2016

Coverage Status

Coverage remained the same at 88.004% when pulling 02495b3 on nilgoyyou:broken_builds_fmin into eec397d on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 18, 2016

Current coverage is 85.46% (diff: 100%)

Merging #1151 into master will not change coverage

@@             master      #1151   diff @@
==========================================
  Files           214        214          
  Lines         24917      24917          
  Methods           0          0          
  Messages          0          0          
  Branches       2526       2526          
==========================================
  Hits          21296      21296          
  Misses         2989       2989          
  Partials        632        632          

Powered by Codecov. Last update eec397d...5edcccc

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 18, 2016

I think what you see is what's expected - try_branch sends the patch relative to master, and applies it. I just looked on the machine, the code looks correct, with these patches applied.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 18, 2016

I think I found the problem - the macro for detecting windows should be _WIN32 not WIN32:

diff --git a/src/dpy_math.h b/src/dpy_math.h
index 3f9e227..cfadc34 100644
--- a/src/dpy_math.h
+++ b/src/dpy_math.h
@@ -18,7 +18,7 @@ double dpy_log2(double x)
 #endif
 }

-#if (defined(WIN32) || defined(_WIN64)) && !defined(__GNUC__)
+#if (defined(_WIN32) || defined(_WIN64)) && !defined(__GNUC__)
 #define fmin min
 #endif
@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 18, 2016

I wasn't aware there was a problem, but it seems you're right, _WIN32 is the official way to detect Windows. I'll change it.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Nov 19, 2016

The problem was the error that Ariel was finding with try_branch.py - the compilation is still failing with Python 2.7, at least on Windows 32-bit. It passes with the change to _WIN32.

@arokem

This comment has been minimized.

Member

arokem commented Nov 20, 2016

Yep. Seems to compile fine with this change: http://nipy.bic.berkeley.edu/builders/dipy-bdist32-33/builds/289

@coveralls

This comment has been minimized.

coveralls commented Nov 20, 2016

Coverage Status

Coverage remained the same at 88.004% when pulling 5edcccc on nilgoyyou:broken_builds_fmin into eec397d on nipy:master.

@arokem arokem merged commit e87b327 into nipy:master Nov 20, 2016

4 checks passed

codecov/patch Coverage not affected when comparing eec397d...5edcccc
Details
codecov/project 85.46% (+0.00%) compared to eec397d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.004%
Details

@arokem arokem referenced this pull request Dec 16, 2016

Closed

A few broken builds #1138

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