Fix TypeError in XcodeVersion()

[cclauss]

Fixes: #1917

Recent changes have resulted in improper encoding of versions in the XcodeVersion() function.
This PR is meant to streamline the logic and properly encode versions that have single digit as well as double digit major versions.

Checklist

• npm install && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

[richardlau]

Not really a macOS person but the changes look to be an improvement over what is already there. I'd question the returned format (does it mean the minor and micro bits of the version string cannot be more than a single digit?) but it looks like it's used to populate Info.plist:

node-gyp/gyp/pylib/gyp/xcode_emulation.py

Lines 1092 to 1093 in e610838

	cache['DTXcode'] = xcode
	cache['DTXcodeBuild'] = xcode_build

I haven't been able to find an authoritive reference for those keys with a quick web search.

[LavaToaster]

I'm not sure that this is a solution to the issue I posted:

If XcodeVersion returns a tuple, and gets used in a comparison function, like these:

• node-gyp/gyp/pylib/gyp/xcode_emulation.py

  Line 858 in e610838

  if XcodeVersion() < '0900':

• node-gyp/gyp/pylib/gyp/xcode_emulation.py

  Line 1524 in e610838

  if XcodeVersion() >= '0500' and not env.get('SDKROOT'):

Then surely it will error, like it did for me.

By comparison, if you look at the others uses of it, where I believe its used properly. They properly deconstruct the tuple for use in comparisons.

• node-gyp/gyp/pylib/gyp/xcode_emulation.py

  Lines 122 to 145 in e610838

  	xcode_version, _ = XcodeVersion()
  	if xcode_version < '0500':
  	XCODE_ARCHS_DEFAULT_CACHE = XcodeArchsDefault(
  	'$(ARCHS_STANDARD)',
  	XcodeArchsVariableMapping(['i386']),
  	XcodeArchsVariableMapping(['i386']),
  	XcodeArchsVariableMapping(['armv7']))
  	elif xcode_version < '0510':
  	XCODE_ARCHS_DEFAULT_CACHE = XcodeArchsDefault(
  	'$(ARCHS_STANDARD_INCLUDING_64_BIT)',
  	XcodeArchsVariableMapping(['x86_64'], ['x86_64']),
  	XcodeArchsVariableMapping(['i386'], ['i386', 'x86_64']),
  	XcodeArchsVariableMapping(
  	['armv7', 'armv7s'],
  	['armv7', 'armv7s', 'arm64']))
  	else:
  	XCODE_ARCHS_DEFAULT_CACHE = XcodeArchsDefault(
  	'$(ARCHS_STANDARD)',
  	XcodeArchsVariableMapping(['x86_64'], ['x86_64']),
  	XcodeArchsVariableMapping(['i386', 'x86_64'], ['i386', 'x86_64']),
  	XcodeArchsVariableMapping(
  	['armv7', 'armv7s', 'arm64'],
  	['armv7', 'armv7s', 'arm64']))
  	return XCODE_ARCHS_DEFAULT_CACHE

• node-gyp/gyp/pylib/gyp/xcode_emulation.py

  Lines 1091 to 1104 in e610838

  	xcode, xcode_build = XcodeVersion()
  	cache['DTXcode'] = xcode
  	cache['DTXcodeBuild'] = xcode_build
  	sdk_root = self._SdkRoot(configname)
  	if not sdk_root:
  	sdk_root = self._DefaultSdkRoot()
  	cache['DTSDKName'] = sdk_root
  	if xcode >= '0430':
  	cache['DTSDKBuild'] = self._GetSdkVersionInfoItem(
  	sdk_root, 'ProductBuildVersion')
  	else:
  	cache['DTSDKBuild'] = cache['BuildMachineOSBuild']

• node-gyp/gyp/pylib/gyp/xcode_emulation.py

  Lines 1129 to 1131 in e610838

  	xcode_version, xcode_build = XcodeVersion()
  	if xcode_version < '0500':
  	return ''

These previews are rendered from your commit.

[cclauss]

@Lavoaster You are correct. Thanks for this very precise review. I have made appropriate changes.

[sam-github]

This looks reasonable to me.

[cclauss]

@bdon

[rvagg]

@gengjiawen these commits needed metadata before merging, PR-URL and Reviewed-By at a minimum, and the middle one is missing the subsystem prefix. Please don't merge using the GitHub UI so this can be avoided.
I'll address it and force-push.

[rvagg]

sorry, there's two commits at fault here, one is from #1937 and this one got squashed (thanks), but they're both missing subsystem, in this case gyp: from the commit messages plus the metadata. We do this mainly for nice changelogs.

[rvagg]

see 9c0f340 and bb2eb72 for the modified forms, force pushed to master

[gengjiawen]

@gengjiawen these commits needed metadata before merging, PR-URL and Reviewed-By at a minimum, and the middle one is missing the subsystem prefix. Please don't merge using the GitHub UI so this can be avoided.
I'll address it and force-push.

Sorry for this trouble. Is https://github.com/nodejs/node-core-utils/blob/master/docs/git-node.md#git-node-metadata can generate the meta info ?

[rvagg]

that's the metadata we need but I don't know if it'll work on this repo, I just do it manually, but keeping to nodejs/node conventions is the aim (also no merge commits)

[gengjiawen]

Hate merge commits too :)