[PATCH] D48309: [sanitizer] Unify and generalize Apple platforms in CMake and lit test configs

[George Karpenkov via Phabricator via llvm-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.

Looks good to me, I think suggestions from @delcypher should be done in subsequent patches.



================
Comment at: test/lit.common.cfg:111
+
   config.available_features.add('ios')
+  if ios_or_iossim == "iossim":
----------------
delcypher wrote:
> We should really fix the name of the  `ios` lit configuration feature. The feature is added for any ios-like platform (i.e. Apple platform that is not macos) but it is really not obvious from the name. It could also be confused with `config.apple_platform` being set to `ios` which means something different. At some point in the future we might add the value `config.apple_platform` as a feature i.e.
> 
> ```
> config.available_features.add(config.apple_platform)
> ```
> 
> This would write tests that can be enabled/disabled based on `config.apple_platform`. However we can't do this if the `ios` feature means something else.
> 
> A possible new name for the `ios` feature could be `apple_embedded_platform` and the logic for setting this feature would simply be
> 
> ```
> if config.host_os == 'Darwin' and config.apple_platform != 'osx':
>   config.available_features.add('apple_embedded_platform')
> ```
> 
> The feature name is a little long so I'd like to hear other ideas but its name should not match any of the possible values for `config.apple_platform`.
> 
> I realise this would be a very invasive change that would require changing lots of tests so if we do this I suggest this is done separately as a follow up commit.
@delcypher while I don't disagree, shouldn't this be done in another review?


================
Comment at: test/lit.common.cfg:120
+  device_id_env = "SANITIZER_" + ios_or_iossim.upper() + "_TEST_DEVICE_IDENTIFIER"
+  run_wrapper = os.path.join(ios_commands_dir, ios_or_iossim + "_run.py")
+  env_wrapper = os.path.join(ios_commands_dir, ios_or_iossim + "_env.py")
----------------
delcypher wrote:
> These scripts are named based on the old naming scheme and really should be fixed. E.g. 
> 
> `ios_run.py` -> `device_run.py`
> `iossim_run.py` -> `device_sim_run.py`
> 
> similarly for the `_prepare`, `_env` and `_compile` suffixed scripts too.
> 
> This would also allow you to rename  the `ios_or_iossim` variable.
> 
> Side note the `ios_commands` directory also could do with a rename at some point.
Again could be done in a subsequent review?


================
Comment at: test/lit.common.cfg:141
+
+  if device_id_env in os.environ: config.environment[device_id_env] = os.environ[device_id_env]
   config.substitutions.append(('%run', run_wrapper))
----------------
newline after colon


https://reviews.llvm.org/D48309