980952 - Ability for bootstrap script to blacklist commands

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

The underlying problem is that most Gecko mach commands don't work with B2G. I want to be able to selectively choose which commands the B2G bootstrap script loads, and which ones it ignores.

One way to do this would be to simply not register any mach scripts that contain commands that don't work with B2G. The problem with this approach is that many mach scripts have both commands that work with B2G and ones that don't. This approach isn't granular enough.

Another approach (which is the current approach) is to only load commands that have conditions applied to them. But this is also not ideal, because what if a command that doesn't work with B2G also wants a condition for some other reason?

I'd like to introduce a simple 'command_blacklist' property the bootstrap script can set that will just ignore any command names that appear in that list. This will allow me to remove the 'require_conditions' property from the b2g bootstrap. In fact, I don't think the 'require_conditions' property is very useful in general, I should just remove it from mach completely.

Comment 1

[Andrew Halberstadt [:ahal]]

Hmm, now I'm wondering if a whitelist would be better.. there seem to be a lot more gecko commands that we want disabled than we do enabled. I don't think it really hurts to support both, the change is very easy.

Comment 2

[Andrew Halberstadt [:ahal]]

Actually no, a whitelist is a bad idea because there would be no way to differentiate between commands implemented in gecko and commands implemented in b2g. Then the bootstrap script would need to be updated every single time a command is added.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: add ability to blacklist commands from the bootstrap script

Comment 4

[Gregory Szorc [:gps]]

I'm not going to have time to review until at least next week.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8387721 [details] [diff] [review]
add ability to blacklist commands from the bootstrap script

Review of attachment 8387721 [details] [diff] [review]:
-----------------------------------------------------------------

My initial reaction is I'm fundamentally opposed to a blacklist - at least as implemented.

I think the existing approach of having conditions that must pass to enable a command is a good idea. Correct me if I am wrong, but the problem with b2g is that many commands still don't have conditions and thus they are showing up when they shouldn't be.

I would prefer:

* Expanding the command conditions to cover more (all?) commands
* Consider requiring command conditions on every command
* A mode that ignores commands with no conditions
* And if it's still needed, a blacklist

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Gregory Szorc [:gps] from comment #5)
> My initial reaction is I'm fundamentally opposed to a blacklist - at least
> as implemented.

Conceptually I think of disabled (via condition) as meaning you are in the wrong state to run this command, but the command is supported by your source tree for some state.

I think of blacklisted as meaning this command does not work in your current source tree, and never will work no matter what state you are in.

> I think the existing approach of having conditions that must pass to enable
> a command is a good idea. Correct me if I am wrong, but the problem with b2g
> is that many commands still don't have conditions and thus they are showing
> up when they shouldn't be.

No, currently B2G mach ignores commands that don't have conditions. The problem I am trying to solve here is to not pollute |mach help| with commands that will never ever be able to run in a B2G context. I'm trying to avoid developers saying "Hey, there's a cppunittest command in mach, but it's not letting me run it!"

> I would prefer:
> 
> * Expanding the command conditions to cover more (all?) commands
> * Consider requiring command conditions on every command
> * A mode that ignores commands with no conditions
> * And if it's still needed, a blacklist

This is what we are doing currently and I would be in favour of adding conditions to all commands. But in order to do this with B2G we'd need to do some hacky things like using a "lambda x: True" condition for commands like |mach uuid|. We'd also need to apply a condition that explicitly ignores B2G to all commands that don't work there. And even then the command will still show up in |mach help|.

Anyway, it's possible that I'm making too much of a big deal about having irrelevant commands show up in B2G's |mach help|. If you still think a blacklist is a bad idea, I'll WONTFIX this bug and work on adding conditions to the commands that are relevant to B2G.

Comment 7

[Andrew Halberstadt [:ahal]]

Actually another solution to my problem would be to whitelist gecko commands in the B2G bootstrap script. This would require a change to the B2G repo anytime we wanted to add a new command, which I was trying to avoid, but it isn't that terrible. It also wouldn't require any changes to mach core.

Comment 8

[Andrew Halberstadt [:ahal]]

Sorry, ignore my last comment. The reason that won't work is because I can only whitelist at the script level, not the command level.

Comment 9

[Gregory Szorc [:gps]]

Can we just hide unavailable commands from mach help?

Comment 10

[Andrew Halberstadt [:ahal]]

I guess so.. I don't really see much difference between hiding from help and blacklisting, but I'll get a new patch up.

Comment 11

[Gregory Szorc [:gps]]

The output from mach help is getting pretty long and unwieldy. While that's arguably a good problem to have (more mach usage!), making the UI for help better is probably overdue.

Another solution is to expand the compatibility check to denote between "this command will never be compatible with your environment" and "you need to do something to make this command compatible." I reckon you could expand the API to return a rich data type instead of merely a bool and then we could retrofit the compatibility checks to be b2g aware?

Comment 12

[Andrew Halberstadt [:ahal]]

(In reply to Gregory Szorc [:gps] from comment #11)
> Another solution is to expand the compatibility check to denote between
> "this command will never be compatible with your environment" and "you need
> to do something to make this command compatible." I reckon you could expand
> the API to return a rich data type instead of merely a bool and then we
> could retrofit the compatibility checks to be b2g aware?

Yeah, that's not a bad idea. If we were so inclined we could even have the bootstrap scripts define a hook to evaluate the rich data type returned by the conditions. Then gecko and B2G could evaluate the same condition results in different ways. Though this is probably putting the horse before the cart.

Comment 13

[Andrew Halberstadt [:ahal]]

Er, cart before the horse :)

Comment 14

[Andrew Halberstadt [:ahal]]

This bug as filed was rejected. If we want to pursue this a different way we can file a new bug. For now things seems to be working fine.