Pass product module name into test_rule

[dostrander]

This will need to wait until this is merged

[jerrymarino]

Should this use kwargs.module_name?

[dostrander]

Yes! I thought this was a thing but I couldn't find it initially

[mattrobmattrob]

Have we solved additions to rules_apple like this in a backwards compatible way in the future or just assume you'll need to bump to be on this change or newer?

[jszumski]

Would adding "module_name" to _IOS_TEST_KWARGS accomplish the same thing while keeping all the important keys in one place?

[dostrander]

Yeah it should. Tried it at first but it failed for a way i didn't understand and din't have to time to debug. I actually didn't mean to push this up but was int he wrong repo :). I need to debug why it wasn't working and hopefully get it to work

[dostrander]

So this wasn't able to be done because the kwargs["module_name"] was also used in apple_library our current implementation of _IOS_TEST_KWARGS pops them off so they aren't passed into apple_library. I added a comment explaining in code so it's not as confusing int he future

[dostrander]

Have we solved additions to rules_apple like this in a backwards compatible way in the future or just assume you'll need to bump to be on this change or newer?

@mattrobmattrob I was thinking of looking into some patching for this PR as well but i'm not sure how future-proof that is

[dostrander]

@mattrobmattrob @jerrymarino PTAL at how I plan on doing patching. LMK if there is something else we may want to do

[jerrymarino]

Filtering looks fine - my main comment is on maintaining patch files for rules_apple bumps.. Do you have PRs linked for that? We can take the other approach for a subset of rules_apple if they aren't upstreamable.

[jerrymarino]

Also are there concerns the rules_apple patch will break for people even with landing this? I gave it a stamp assuming we'll fix it - but it might cause an issue.

[jerrymarino]

Have we solved additions to rules_apple like this in a backwards compatible way in the future or just assume you'll need to bump to be on this change or newer?

➕ to @mattrobmattrob comment on this

[jerrymarino]

I think this will break other people that have a different version of rules_apple ( similar of @mattrobmattrob's comment )

[jerrymarino]

We already keep a fork ofxctestrunner for our patches from time to time. To better integrate with how we do things, and to integrate with github workflows I'd like to use that instead of patch files: https://github.com/bazel-ios/xctestrunner - similar for rules_apple as well.

This said - we'll still need to make it backwards compatible.

[mattrobmattrob]

FWIW, I don't want to block a merge because of backwards compatibility, necessarily. Just want it to be a conversation. 👍 We are essentially on trunk so it shouldn't cause any issues for us.

[jerrymarino]

The "backwards compatibility" issue I encountered was the PR seemed to require me to use the rules_apple version generated by the patch: in other words, if it needs argument module_name which doesn't exist outside of the patches. So if you have a different version of rules_apple than rules_ios gives you it won't work.

[jerrymarino]

@dostrander if you want to add an if statement in a macro - I came up with a mechansim to add this:
#628

load("@rules_ios_cfg//:config.bzl", "using_internal_rules_apple")

if using_internal_rules_apple:
   # some
else:
   # other

We might want this anyhow - but let me know what you're thoughts are. I added some suggestions to simplify rules_apple in that review BTW - both which can be backwards compatible.

[dostrander]

Declining this as I've made a better fix in rules_apple itself. May need to open up a new PR to use that and xctesturnner fixes