godotengine / godotengine/godot-cpp

SConstruct host_platform

Open
#213 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
2.7k
Forks
809
Avg merge
1d 3h
Merged PRs (30d)
8

Description

Looking at the current SConstruct file (after #212), it seems like the `host_platform` value could potentially raise an error even before the `platform` parameter is evaluated. If you are not on a valid platform, but do supply a platform= assignment, then you'd end up with an error message saying you need to specify the platform because it couldn't be detected (which doesn't make any sense). I think we should not raise an error until after the parameter has been collected and evaluated. We also might consider changing the error message itself since our code only handles the 'linux', 'osx', and 'windows' cases anyway. It should instead inform the user of what values are permitted. Either that, or we need to raise an error in the latter if statement sequence to tell users that "build options are undefined for the [automatically?] selected platform: ".

So...

1. Change the following section:

# Try to detect the host platform automatically
# This is used if no `platform` argument is passed
if sys.platform.startswith('linux'):
host_platform = 'linux'
elif sys.platform == 'darwin':
host_platform = 'osx'
elif sys.platform == 'win32':
host_platform = 'windows'
else: # MOVE THIS LINE TO A SEPARATE `if not env['platform']:` LINE AFTER THE PARAM EVALS BELOW
raise ValueError('Could not detect platform automatically, please specify with platform=')

opts = Variables([], ARGUMENTS)

opts.Add(EnumVariable('platform', 'Target platform', host_platform,
allowed_values=('linux', 'osx', 'windows'),
ignorecase=2))

2. Add this to the end of the other if sequence:

if env['platform'] == 'linux':
...
elif env['platform'] == 'osx':
...
elif env['platform'] == 'windows':
...
# ADD THIS LINE
else:
raise ValueError('Build options are not defined for platform: ' + env['platform'])

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.