do not set deprecated target args

[matt-sm]

I was unable to set a setup arg via target_args, because the value would be overridden by the empty value in setup_args. And if I tried to set setup_args I got a fail saying it was deprecated.

[jjmaestro]

You are right that my code was wrong: if you set target_args['setup'] and don't set ctx.attr.setup_args, the latter will have [] as default and the if args_ will be False, so the fail will be correctly skipped, but then, the last line in the for-loop will override the target_args['setup'] you set with the [] from ctx.attr.setup_args. Sorry!

Having said that, I don't think this PR addresses or fixes the actual problem. IMHO it's just side-stepping it by deleting a line of code.

I think the proper fix is:

for target_name, args_ in deprecated:
    if args_:
        if target_name in target_args:
            fail("Please migrate '{t}_args' to 'target_args[\"{t}\"]'".format(t = target_name))
        target_args[target_name] = args_

This will be a no-op if args_ is the default [] for each of the args it checks (all of them are string_list without any default, so they default to []). If args_ is set, it will only set the arguments in target_args after checking that there's no conflict for that argument. And if there was, it fails informing the user to migrate to the new target_args.

[matt-sm]

Thanks for responding

I don't think this PR addresses or fixes the actual problem

I suppose it depends on what you're defining as the problem :) The problem I was trying to solve was:

target_args = {"setup": ["--foo=bar"]},

was completely broken, and if as a side effect we deprecate setup_args I was fine with that. But yes it breaks the case where you want to mix/match. I imagine I must be one of the few people using target_args as there have been no issues raised.

But anyway, I updated the branch to your change because it is more flexible. Perhaps one day this may even be merged to main :)

[jjmaestro]

I suppose it depends on what you're defining as the problem :)

Of course 😆 "Obviously", it's deprecating the old attrs while introducing the new one! 😄 So, to do that, we do need to update target_args with whatever is in the old args :)

I imagine I must be one of the few people using target_args as there have been no issues raised.

Yeah, I only added it to have the flexibility of having other Meson targets available (e.g. the introspection). I'm pretty sure 99.9% is only using the old args :D

But anyway, I updated the branch to your change because it is more flexible.

Thanks, LGTM! 🙌

Perhaps one day this may even be merged to main :)

Well, it's a fix, my PR was broken and this fixes it, so I think @jsharpe will definitely consider merging it!

[jjmaestro]

LGTM!

[matt-sm]

@cerisier this is a fairly straight forward bug fix, if you're able to review

[cerisier]

Fine with me. I'll discuss making a release that removes the deprecated fields. It's a been a while now.