rules_apple icon indicating copy to clipboard operation
rules_apple copied to clipboard

Command line --minimum_os_version is not supported for macOS rules

Open nikhilm opened this issue 5 years ago • 9 comments

I'm using macos_xpc_service. Based on #196 I expected to be able to set --minimum_os_version=10.10 in my .bazelrc and have it set workspace wide. I also set --macos_minimum_os just in case. However, macos_xpc_service has this attr marked as required. If I change a forked copy of the rules to make them not required, I still get this transition failure.

ERROR: /private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/transition_support.bzl:50:5: OptionsParsingError for option 'macos_minimum_os': Dotted version components must all be of the form \d+([a-z0-9]*?)?(\d+)? but got '<built-in function target_apple_en
v>'                                                                                                                                                                                                                                                                                                                        
ERROR: Errors encountered while applying Starlark transition                                                                                                                                                                                                                                                               
ERROR: /private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/transition_support.bzl:50:5: OptionsParsingError for option 'macos_minimum_os': Dotted version components must all be of the form \d+([a-z0-9]*?)?(\d+)? but got '<built-in function target_apple_en
v>'                                                                                                                                                                                                                                                                                                                        
ERROR: Errors encountered while applying Starlark transition                                                                                                                                                                                                                                                               
ERROR: /private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/BUILD:489:18: in environment_plist rule @build_bazel_rules_apple//apple/internal:environment_plist_macos:                                                                                          
Traceback (most recent call last):                                                                                                                                                                                                                                                                                         
        File "/private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/environment_plist.bzl", line 39, column 23, in _environment_plist                                                                                                                           
                legacy_actions.run(                                                                                                                                                                                                                                                                                        
        File "/private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/utils/legacy_actions.bzl", line 115, column 45, in _run                                                                                                                                     
                actions.run(**_kwargs_for_apple_platform(                                                                                                                                                                                                                                                                  
        File "/private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_rules_apple/apple/internal/utils/legacy_actions.bzl", line 66, column 93, in _kwargs_for_apple_platform                                                                                                                
                action_execution_requirements = apple_support.action_required_execution_requirements(ctx)                                                                                                                                                                                                                  
        File "/private/var/tmp/_bazel_nikhilm/f7e4a634181022223ae0be799fce6716/external/build_bazel_apple_support/lib/apple_support.bzl", line 187, column 5, in _action_required_execution_requirements                                                                                                                   
                def _action_required_execution_requirements():                                                                                                                                                                                                                                                             
Error: _action_required_execution_requirements() does not accept positional arguments, but got 1 

It would be nice to make this attribute optional and specify it workspace wide using.

nikhilm avatar Oct 08 '20 18:10 nikhilm

What version of rules_apple / apple_support are you using, sounds like you need to update apple_support

keith avatar Oct 08 '20 20:10 keith

Thanks! The root cause was definitely being on an older version of apple_support. That said, after updating that to 0.9.0, there is still some unintuitive behavior (I'm on rules_apple 0.20.0, not master):

  1. macos_xpc_service still has the attr required, so I need to set it to an empty string. I'm happy to submit a PR to fix this.
  2. It seems --minimum_os_version has no effect, but --macos_minimum_os does. Which makes sense based on the transitions, but... I don't know why the former exists then :). This is certainly a bug I'll file upstream.

This can be closed regardless. Thanks!

nikhilm avatar Oct 09 '20 16:10 nikhilm

Ah this just seems to be a non-Apple flag https://github.com/bazelbuild/bazel/commit/f463dec7a03

nikhilm avatar Oct 09 '20 16:10 nikhilm

Yea I actually hadn't seen that flag before, seems fair to submit something upstream about removing it if it really is unused, but my bet is things are used internally, or it wouldn't be removed since folks theoretically could be using it.

keith avatar Oct 09 '20 20:10 keith

Or we could try to make that apply to iOS / macOS compiles as well

keith avatar Oct 09 '20 20:10 keith

Re-opening because we noticed some issues after updating to master (229680b8f99382889fd410259cd141ba61a667d9).

minimum_os_version is now required on each target due to the addition of a transition and limitations on how transitions work in Bazel. This seems to be because of https://github.com/bazelbuild/rules_apple/blob/229680b8f99382889fd410259cd141ba61a667d9/apple/internal/transition_support.bzl#L59.

Attempting to leave it as an empty string results in

ERROR: <PII>/external/build_bazel_rules_apple/apple/internal/transition_support.bzl:64:5: OptionsParsingError for option 'macos_minimum_os': Dotted version components must all be of the form \d+([a-z0-9]*?)?(\d+)? but got ''
ERROR: Errors encountered while applying Starlark transition
ERROR: Analysis of target '//app/mac:xpc_services' failed; build aborted: com.google.devtools.build.lib.analysis.starlark.StarlarkTransition$TransitionException: Errors encountered while applying Starlark transition

I tried introducing a change which would access the XcodeVersionConfig from the rule attrs passed to the transition, but a transition is unable to access the providers of a Label typed rule attr to access the default XcodeVersionConfig and use that to get the minimum_os_version. A Label type attr is not "resolved" but just seen as a Label.

nikhilm avatar Oct 23 '20 16:10 nikhilm

cc @segiddins who was just looking at this as well. But I think the workaround is to pass the minimum_os_target on each?

keith avatar Oct 27 '20 00:10 keith

I poked around at this a bit more. Just leaving my understanding here for others.

First I set mandatory = False for minimum_os_version attributes everywhere, just to relax that complaint.

In _min_os_version_or_none above, attempting to return None doesn't work. No LC_VERSION_MIN_MACOSX is set on the resulting executable. Attempting to return the settings value as-is doesn't work, since the incoming DottedVersion instance cannot be returned as DottedVersion does not inter-op to Starlark from Java.

This means simply relaxing the mandatory requirement will not work. Based on the Bazel I know, setting a project wide minimum version seems impossible right now. This is non-ideal because defining it according to the approaches specified in #196 has the potential for divergence from the version set in .bazelrc and used by other (non-rules_apple) rules.

nikhilm avatar Dec 30 '20 10:12 nikhilm

Picking up the pieces from @nikhilm on this as we try again to update our rules_apple to get it to work on our version of Bazel 4.0.

It looks like on Bazel master (https://github.com/bazelbuild/bazel/commit/204de944fae75c6d1db9ef306e9000d71164379e, doesn't appear to be in any of the 4.x releases yet, but that's okay), there's a commit that makes the DottedVersion.Option instance a Starlark value, so that we can manipulate it appropriately from settings (similar to the approach @nikhilm described in the last comment).

With that in mind, would it now make sense to:

  • Make minimum_os_version not mandatory on rules.
  • Use attr.minimum_os_version if exists.
  • Otherwise pass through settings["//command_line_option:{platform}_minimum_os"] if that exists.
  • Fallback to None?

This seems like it would let us set a project wide minimum version but still offer the ability to override if needed, which would make it easier to keep things in sync.

Sakawa avatar Apr 03 '21 01:04 Sakawa