Skip to content

Conversation

brentleyjones
Copy link
Contributor

@brentleyjones brentleyjones commented Jun 27, 2022

Followup to #586. Part of #415.

  • No longer set C related flags for Swift targets, as they aren't used and are instead passed to OTHER_SWIFT_FLAGS with -Xcc prefixes
  • Set Swift PCM flags exactly as rules_swift does
  • Correctly retrieve copts for objc_library targets by checking for ObjcProvider and changing the action that is queried based on that (this check will have to change once ObjcProvider is removed)
  • Only set objc based flags for objc based actions
  • Stop uniquing flags, as Bazel doesn't either

- No longer set C related flags for Swift targets, as they aren't used and are instead passed to `OTHER_SWIFT_FLAGS` with `-Xcc` prefixes
- Set Swift PCM flags exactly as rules_swift does
- Correctly retrieve copts for `objc_library` targets by checking for `ObjcProvider` and changing the action that is queried based on that (this check will have to change once `ObjcProvider` is removed)
- Only set objc based flags for objc based actions
- Stop uniquing flags, as Bazel doesn't either
@brentleyjones brentleyjones force-pushed the bj/apply-compiler-options-the-same-as-bazel branch from ab9f0b1 to 9c73268 Compare June 27, 2022 16:59
Copy link
Contributor

@BalestraPatrick BalestraPatrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally in a couple of projects and didn't find regressions. LGTM!

return base_pcm_flags + pcm_defines

# Lifted from rules_swift, to mimic its behavior
def _swift_command_line_objc_copts(*, compilation_mode, objc_fragment):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda thought to do this but it seemed strange to rip it out, looks like there is really no other way though to make sure we have parity

"-fstack-protector-all",
"-g",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Its only filtered from the Swift PCM side. It's actually set on the Obj-C compiling side.

@brentleyjones brentleyjones merged commit d5fa4fb into main Jun 27, 2022
@brentleyjones brentleyjones deleted the bj/apply-compiler-options-the-same-as-bazel branch June 27, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants