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

(FACT-959) Incorrect processor counts on Windows #1229

Closed
wants to merge 4 commits into from

Conversation

@jasminen
Copy link

jasminen commented Nov 22, 2015

Facter is showing the CPU socket count on processorcount fact which should be the the logical count and not the physical count (which is already represented by the physicalprocessorcount fact).
Apparently it is a bug in the proc.respond_to?(:NumberOfLogicalProcessors) which always returns false since the NumberOfLogicalProcessors is not a regular method but an OLE method. Therefore it should use the ole_respond_to? method (WIN32OLE class).

Facter is showing the CPU socket count on processorcount fact which should be the the logical count and not the physical count (which is already represented by the physicalprocessorcount fact).
Apparently it is a bug in the proc.respond_to?(:NumberOfLogicalProcessors) which always returns false since the NumberOfLogicalProcessors is not a regular method but an OLE method. Therefore it should use the ole_respond_to? method (WIN32OLE class).
@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Nov 23, 2015

@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Nov 23, 2015

The 2.x branch is still open to fixes, and this looks correct. However it appears some spec tests need to be updated with this change.

@kylog

This comment has been minimized.

Copy link
Contributor

kylog commented Nov 23, 2015

@MikaelSmith do we know if facter 3 does The Right Thing in this regard?

@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Nov 23, 2015

Yes, the WMI call implementation is completely different and correctly queries for NumberOfLogicalProcessors.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Nov 23, 2015

@jasminen can you update specs for this? Thanks!

@joshcooper

This comment has been minimized.

Copy link
Member

joshcooper commented Nov 23, 2015

Thanks @jasminen! It looks like this has been broken since 0da8bd7 when I updated the code to support ruby 1.9.

jasminen added 2 commits Nov 25, 2015
Update the spec according to the processor count fix.
jasminen
Replaced "NumberOfLogicalProcessors" with :NumberOfLogicalProcessors
@jasminen

This comment has been minimized.

Copy link
Author

jasminen commented Nov 25, 2015

@ferventcoder, I've updated the specs accordingly. Sorry for that :)
Thanks.

@@ -221,6 +221,7 @@ def load(procs)
before :each do
proc = stubs 'proc'
proc.stubs(:Name).returns("Intel(R) Celeron(R) processor")
proc.stubs(:ole_respond_to?).with(:NumberOfLogicalProcessors).returns(false)

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Nov 25, 2015

Contributor

Additionally you should ensure that proc.NumberOfLogicalProcessors is not called.

This comment has been minimized.

Copy link
@joshcooper

joshcooper Dec 15, 2015

Member

I don't think we need an explicit never expectation since it's a stub object and it will raise if any other methods (besides Name) are called on it.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Nov 25, 2015

@jasminen Thanks for updating the specs. I've added at least one comment that I think will help ensure the code is working properly.

Also when you update this time, would you mind squashing the commits together into one commit? If you need help on this, let us know.

@jasminen

This comment has been minimized.

Copy link
Author

jasminen commented Nov 29, 2015

@ferventcoder Thanks for the comment. I am not sure this addition is necessary since if the proc.NumberOfLogicalProcessors will be called the test will fail on unknown method because i am not stubbing it in 2003. This also was the logic before i changed the specs, i only added the response of the ole_respond_to? method. WDYT?
Also, can you please help me understand how to squash my commits into one commit? Thanks :)

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Nov 29, 2015

@jasminen For specs, I'm stating you should ensure that the function is never called. That is a proc.expects(:function_name).never (I believe)

@joshcooper joshcooper closed this Dec 15, 2015
@joshcooper joshcooper reopened this Dec 15, 2015
@joshcooper

This comment has been minimized.

Copy link
Member

joshcooper commented Dec 15, 2015

I thought closing and reopening would rekick appveyor, but I just remembered we don't have it enabled on old branches. We'll need to manually very this on windows before merging.

jasminen
@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Dec 22, 2015

Closed in favor of squash and rebase in #1242.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.