Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#2172 closed task (complete)

Add support for OS X in b10-showtech

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20120821
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 3
Total Hours: 3 Internal?: no

Description (last modified by muks)

Add support for OS X in b10-showtech. This is a sub-ticket of #2085.

Subtickets

Attachments (2)

sysinfo-distro.diff (5.5 KB) - added by jinmei 8 years ago.
sysinfo-route.diff (3.6 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by muks

  • Description modified (diff)
  • Summary changed from add support for OS X in b10-showtech to Add support for OS X in b10-showtech

comment:2 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:3 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

ready for review; it's mostly the same as for FreeBSD, except for the way to find swap usage. AFAICT, 'buffers' and 'cache' are a typical linux thing, but perhaps the reviewer will prove me wrong :p

comment:4 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:5 follow-up: Changed 8 years ago by jinmei

sysinfo.py

  • (although not specific to this branch) I don't think it a good practice to have Linux specific notions like mem_cached, mem_buffers, or get_platform_distro() at the top level. It seems to be a typical example of the "the world is Linux" mindset. IMO we should either introduce a higher abstraction for these that can apply more system independently or move them to Linux specific code.
  • The implementation seems to share much of the code with the FreeBSD version. I'd unify them as much as possible based on the DRY principle.
  • at least on my MBP kern.smp.active isn't available:
    % sysctl kern.smp.active
    second level name smp in kern.smp.active is invalid
    
    and I don't even know if it's possible to activate/deactivate an SMP support on MacOS X.
  • I don't think it a good idea to have a hardcoded page_size default. If it's not known (which should be very rare or indicate we need to update the parser script), IMO we should also leave parameters that depend on it unknown.
  • Likewise (and although it's not specific to the MacOS version), I'd generally avoid using magic numbers like "-1" to represent "unknown", "unavailable", etc, because magic numbers make the intent of the code more vague and can often lead to unexpected bugs (e.g.) as a result of automatic conversion. Python has a perfection representation for this type of concepts: None. I'd have get_mem_free, etc return None when the value is unknown and convert it to corresponding string at the main program, or we could simply have get_xxx return the corresponding string if we only use them for printing purposes.

sysinfo_test.py

  • _my_osx_os_sysconf: what's the magic number of 91? Arbitrary chosen? As a magic number hater, I'd rather define a constant variable with commenting the intent and use the variable throughout the file. Also, using the same constants for different systems is probably not a good idea anyway as we wouldn't be able to catch some type of regression.
  • _my_osx_subprocess_check_output: this generally seems to be the same as that for FreeBSD. If we unify the main code (see above), it's probably better to unify the test case, too. Or, if we want to separate the two test cases (which is not necessarily a bad idea), it's probably better to use different faked constants.
  • _my_osx_subprocess_check_output: maybe related to the previous point, 'daemon' doesn't seem to be very compatible with OS X.
        if command == ['hostname']:
            return b'daemon.example.com\n'
    
  • test_sysinfo_osx: I've fixed a trivial typo in a comment. And, this seems to suggest we might rather unify these test_xxx's and switch from it depending on the system type, rather than repeating the same logic for every case.
  • test_sysinfo_osx: not really specific to this branch, but it'd be safer to restore the original functions in tearDown().

comment:6 Changed 8 years ago by jinmei

  • Sub-Project changed from DNS to Core

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte
  • Type changed from defect to task

comment:8 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

sysinfo.py

  • (although not specific to this branch) I don't think it a good practice to have Linux specific notions like mem_cached, mem_buffers, or get_platform_distro() at the top level. It seems to be a typical example of the "the world is Linux" mindset. IMO we should either introduce a higher abstraction for these that can apply more system independently or move them to Linux specific code.

We can go for a higher-abstraction layer (or maybe even a different kind of abstraction, by defining how to get things and which things to get for which operating systems). But to keep the changes reasonable, I changed it a little bit differently; values set to None are simply not printed.

  • The implementation seems to share much of the code with the FreeBSD version. I'd unify them as much as possible based on the DRY principle.

done

  • at least on my MBP kern.smp.active isn't available:
    % sysctl kern.smp.active
    second level name smp in kern.smp.active is invalid
    
    and I don't even know if it's possible to activate/deactivate an SMP support on MacOS X.

one of the things no longer printed (OSX is not the only one where it has no meaning)

  • I don't think it a good idea to have a hardcoded page_size default. If it's not known (which should be very rare or indicate we need to update the parser script), IMO we should also leave parameters that depend on it unknown.

Done

  • Likewise (and although it's not specific to the MacOS version), I'd generally avoid using magic numbers like "-1" to represent "unknown", "unavailable", etc, because magic numbers make the intent of the code more vague and can often lead to unexpected bugs (e.g.) as a result of automatic conversion. Python has a perfection representation for this type of concepts: None. I'd have get_mem_free, etc return None when the value is unknown and convert it to corresponding string at the main program, or we could simply have get_xxx return the corresponding string if we only use them for printing purposes.

Changed all defaults that were no strings to None

fixed

sigh :) Fixed

sysinfo_test.py

  • _my_osx_os_sysconf: what's the magic number of 91? Arbitrary chosen? As a magic number hater, I'd rather define a constant variable with commenting the intent and use the variable throughout the file. Also, using the same constants for different systems is probably not a good idea anyway as we wouldn't be able to catch some type of regression.

Done

  • _my_osx_subprocess_check_output: this generally seems to be the same as that for FreeBSD. If we unify the main code (see above), it's probably better to unify the test case, too. Or, if we want to separate the two test cases (which is not necessarily a bad idea), it's probably better to use different faked constants.

Done, check_output is now roughly following the same hierarchy as the sysinfo classes themselves. Did the same for the actual testing code

  • _my_osx_subprocess_check_output: maybe related to the previous point, 'daemon' doesn't seem to be very compatible with OS X.
        if command == ['hostname']:
            return b'daemon.example.com\n'
    

Since these are now in one place, changed it to 'test.example.com'

  • test_sysinfo_osx: I've fixed a trivial typo in a comment. And, this seems to suggest we might rather unify these test_xxx's and switch from it depending on the system type, rather than repeating the same logic for every case.

Oh right just mentioned I had done this.

  • test_sysinfo_osx: not really specific to this branch, but it'd be safer to restore the original functions in tearDown().

Also done.

And while I was at it, I also fixed another big annoyance; it had all this code to fake system call results, then ran only the tests for the current operating systems. Changed it to always run all tests on all systems (had to add one more fake system call to do that).

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

The revised version basically looks okay.

I have a few final suggestions. If you agree on these, please apply
them and merge; if you don't agree and don't want to include them, I'm
okay with skipping them at least for this branch, in which case please
just merge; if you don't agree but want to continue the discussion,
please do so.

  • SysInfoOSX constructor: if setting _mem_total fails due to exception hw.physmem will be incorrectly used. I'd reset it to None before trying:
            self._mem_total = None
            try:
                s = subprocess.check_output(['sysctl', '-n', 'hw.memsize'])
                self._mem_total = int(s.decode('utf-8').strip())
    
  • I don't think it makes sense to include the concept of 'distro/distribution' for anything other than Linux. As far as I know the concept of 'distribution' is Linux specific, and at the very least BSDs don't have that concept. And, in fact, what we show in the 'distribution' row for BSDs is just redundant (platform name and version, which is already shown). As I argued in my previous comment, for longer term I think we should remove this concept from the top level, but at the moment I propose simply skipping this info for BSDs. I'm attaching a proposed diff for that.
  • totally unrelated to this ticket, but why do we use route(8) to get access to routing table for OpenBSD instead of netstat -r? As far as I know the latter works just like in other BSDs, and it'd make the implementation and tests simpler if we consistently use netstat. I'm attaching a proposed diff for this change.

Changed 8 years ago by jinmei

Changed 8 years ago by jinmei

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 3

comment:12 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

I took your suggestion for setting mem_total to None, and I have no problems with the diffs, to I applied them.

Merged the whole bunch, closing ticket.

comment:13 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4
Note: See TracTickets for help on using tickets.