Select subset of flags on startup / conditionally allow flags

[sharper]

Select subset of flags on startup / conditionally allow flags

[GoogleCodeExporter]

An interesting idea!

I wonder if there's a way to implement it without having to widen the flags API 
-- that is, to do it with the existing API.  In particular, I'd be glad not to 
add more DEFINE_* macros.  (There have been several kinds suggested so far, and 
if we did them all, we'd soon have exponential growth!  So I've been holding 
the line at adding none.)

I'm not sure your code snippet works:
        if (argc >= 2 && !strcmp(argv[1], "print")) {
            REGISTER_CONDITIONAL_int32(lines);
       }

What if I do
   git --some_global_flag print --some_print_specific_flag foo
?  I think what you really want is something that works like this:

   int first_arg = google::ParseCommandLineFlags(..., option_that_says_not_to_reorder_flags);
   if (argv[first_arg] == 'print') {
      register_new_flags;
      google::ParseCommandLineFlags(argv+first_arg, argc-first_arg, ...);
   }

We've seen requests for this kind of mode before in python-gflags.  I don't 
remember what ended up happening there.

One way to handle that would be to do something less general than your 
proposal: you can register a new kind of FlagValidator that takes a string 
("print" in your example).  When doing this, you're saying that argv will have 
the form
   foo --global_flags <some_string> --local_flags
and ParseCommandLineFlags() will handle that appropriately.  It will use the 
FlagValidator to register the proper flags based on <some_string>.

Probably we wouldn't want to call this new thing a FlagValidator, but it would 
work similarly.  What do you think?

Original comment by [email protected] on 19 Jul 2011 at 11:26

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Agreed, my code snipped needed some improvement -- global flags before the 
"print" COMMAND are typically required.  

For simplicity, one can describe the discussed functionality in two separate 
parts:
  (1) Allow registration of flags at runtime.
  (2) Use the FlagValidator-thing (I like this clever idea) to register flags _during_ ParseCommandLineFlags().


For (1), I should have given the simpler example of multiple symlinks to a 
single executable:
   if (basename(argv[0]) == string("symlink_called_print"))
        register_print_flags();
   google::ParseCommandLineFlags(...);

For this to work, somehow one would need to define the global FLAGS_* variable, 
without having it in the set of allowed flags.  Not polluting the API with 
additional macros makes sense!  Did you have something like the following in 
mind?  
    DEFINE_int32(lines, 20, "maximal number of lines to print");
    static const bool lines_dummy = google::MakeConditionalFlag(&FLAGS_line);
    void register_print_flags() {
        google::EnableConditionalFlag(&FLAGS_line);
    }
This would be an attractive API for me.


For (2), maybe one could call this an ArgumentValidator?  One way to use it 
might be:
    // Called during command line parsing.
    // arg_index == 0 corresponds to argv_after_remove_flags[0], etc.
    static bool validate_argument(int arg_index, const char *arg) {
       if (arg_index == 1 && arg == string("print"))
           register_print_flags();
       return true;
    }

    int main(int argc, char **argv) {
        google::RegisterArgumentValidator(validate_argument);
        google::ParseCommandLineFlags(&argc, &argv, true);
        ...
    }
I have no strong opinion regarding (2), and depending on your preferences it 
could be moved into a separate ticket.

Original comment by [email protected] on 20 Jul 2011 at 9:49

[GoogleCodeExporter]

Yeah, I don't have enough experience in this space to know what the right API 
is.

Here's one potential problem: what if two different options have the same flag, 
but they're not the same kind?
   myapp print --filter        // a boolean flag
   myapp upload --filter=x     // a string flag

We can't have one FLAGS_filter in this case.  We either need to
a) support registering flags before ParseCommandLineFlags, or
b) Allow flags to live in namespaces or something similar (so we have 
FLAGS_print::filter, and FLAGS_upload::filter, or the like).

The first has issues of how you define a global flag when you're not at global 
scope (but are instead inside main() or whatever).  The second, besides being 
ugly, has issues to be resolved with naming as well (what if you want to 
support 'myapp --print print'?)

This kind of thing is easy to do in langauges like python, but tricky in C++.  
I think there's going to need to be a pretty detailed design doc before we'd 
start thinking about adding such a feature to gflags.

If it were me, I would solve the problem this way:

1) have a separate file for handling your print command, upload command, etc.
2) define the print-specific flags in print.cc, the upload-specific flags in 
upload.cc, etc.
3) Now, myapp --help will always give help for all commands, but they're 
separated by category (since they're separated by file).  So it's pretty 
readable, hopefully.  You could use the appropriate --help flag (we have so 
many!) to restrict the help to a single file.  You could even add a flag of 
your own for syntactic sugar around that.
4) You could add flag-validators around each flag that verifies that argv[1] 
(or whatever) is appropriate for setting that flag, and complain if not.  That 
is, you can write a validator that makes sure a flag isn't used when it's not 
appropriate for the given command.  This is more annoying than having gflags do 
this validation for you, but it's still possible, and localized in one place, 
which is good.

The downside of this approach is that you can't have flags for two different 
commands share the same name (well, you can, but it starts to get ugly).  The 
upside is it requires no new infrastructure.  If it were me, I'd try it and see 
how it works.  If it works well -- or at least well enough -- we could provide 
some helper routines to make it easier to set up.

Original comment by [email protected] on 21 Jul 2011 at 9:16

• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Agreed, a simple solution would not allow different types for the same 
flag-name (although, arguably this would be an unfriendly way of defining flag 
names/behavior).  As you explain, it is hard/ugly to support this, and it seems 
to me it is pretty independent of a simple solution for flag selection.

The approach I have taken implemented 1) and 2) from your list, and then 
registered only the required flags (using modified macros).  Then, gflags 
automatically keeps help and validation accurate.  This actually works very 
well for me, which is why I wanted to share this.

Given your hints in Comment 1, it seems to me that the following solution would 
work quite nicely (for symlinked binaries), without polluting the API, and 
without restricting further improvements:
  DEFINE_int32(lines, ...);                   // Existing macro, registers flag.
  google::MakeConditionalFlag(&FLAGS_line);   // Marks a flag inactive by default.
  google::EnableConditionalFlag(&FLAGS_line); // Activates flag (called on demand).
Or, for simplicity just call these new functions DisableFlag()/EnableFlag()?

Original comment by [email protected] on 21 Jul 2011 at 9:54

[GoogleCodeExporter]

} Or, for simplicity just call these new functions DisableFlag()/EnableFlag()?

Yeah, I like those names.

Something like this makes sense to me.  Would you be interested in drawing up a 
patch that implements this?

I'm hoping to do a new gflags release next week, so this may not make it into 
the next release, but we could aim for the one after that.  That's probably for 
the best, anyway; I can try the patch internally within google and get some 
more user feedback before making a release.

Original comment by [email protected] on 21 Jul 2011 at 11:00

[GoogleCodeExporter]

This is a fine plan, and I also don't see any reason to rush this for next 
week's release!

Original comment by [email protected] on 22 Jul 2011 at 1:20

[GoogleCodeExporter]

Hi, as discussed, aiming at the release after the next one, here is a patch 
against the current trunk:

  * Add DisableFlag()/EnableFlag() to choose flag subset at runtime (hansres).

Original comment by [email protected] on 26 Jul 2011 at 3:49

Attachments:

• enable_disable_flags.patch

[GoogleCodeExporter]

Thanks -- this patch looks great!  The only comment I have on the code itself 
is the comment in gflags.h.in; I feel it's missing a word:

} +// After defining a flag via DEFINE_bool, etc., it is enabled by
} +// DisableFlag() removes flags from the currently enabled set set; then

Did you mean to say "enabled by default"?

What I'd like to do is gain some experience with this API, and see how well it 
fits in to the use-cases we're envisioning.  I think there are two: one is a 
'busybox'-style app that supports different flags depending on argv[0], and the 
other is a 'driver' app that has different commands, each of which supports 
different flags (I guess these flags come after the command-name?).

It would be great to have small example programs of each type -- maybe as 
unittests.  (We'd have a shell driver, like we do with gflags_unittest.sh, that 
verifies that these small example programs produce the proper --help output, 
and accept or reject --flags as they're supposed to.)  That way, we could look 
and see how pretty or ugly it is to implement the needed functionality using 
these primitives.

Also, now's a good time to get our administrative ducks in a row: can you sign 
the CLA at
   http://code.google.com/legal/individual-cla-v1.0.html
if you've not done so before?  Thanks!

Original comment by [email protected] on 26 Jul 2011 at 9:51

• Changed state: Started

[GoogleCodeExporter]

Thanks for catching the typo in the comment!  I have fixed it in the attached 
patch.

A busy-box style app, switching on argv[0] would look like this:
    // This flag is not available for most commands, disable it by default.
    DEFINE_bool(ascii, false, "Convert end-of-lines using local conventions.");
    static const bool ascii_dummy = google::DisableFlag(&FLAGS_ascii);
    ...
    int main(int argc; char **argv) {
       if (argv[0] == std::string("gzip"))
           google::EnableFlag(&FLAGS_ascii);
       google::ParseCommandLineFlags(&argc, &argv, true);
       ...
    }

For a simple driver app, where the command must be the first argument, main() 
would first check that argc >= 2, and then "argc--, argv++"; otherwise it would 
be identical to the busy-box style app.

The unit test I have added so far is just verifies that flags can be disabled, 
that they are then rejected, and that they can be re-enabled.  I see that there 
would be some value of having more extensive tests, on the other hand, the 
related strip_flags_test shows that this would be a non-trivial amount of 
work/code.

Regarding the CLA, I will be happy to sign it.

Original comment by [email protected] on 27 Jul 2011 at 9:45

Attachments:

• enable_disable_flags.patch.2

[GoogleCodeExporter]

} A busy-box style app, switching on argv[0] would look like this:

Well, that's kinda a toy example, because it only is testing one argv[0] and 
only enabling one flag.  I'd love to see a more realistic example, to see how 
the code looks.  I'm not questioning if the functionality is *sufficient* to do 
what you want, I'm wondering how *practical* it is.  For that we need to see 
what a non-toy example would look like.

Also, having it in a file of its own means we could make a nice unittest out of 
it.  I think that would be worthwhile.

} For a simple driver app, where the command must be the first argument, main()
} would first check that argc >= 2, and then "argc--, argv++"; otherwise it 
would be
} identical to the busy-box style app.

That's only true if the driver app doesn't take any global flags.  In my 
experience that's not the typical case.  If we were going to write up something 
like the 'svn', we'd probably need to call ParseCommandLineFlags twice, once 
for the global flags and once for the command-specific flags.  Would be nice to 
see how that ends up looking.

Original comment by [email protected] on 28 Jul 2011 at 1:00

[GoogleCodeExporter]

New release is out, so I'm upping the priority again.  I'm asking around here 
for feedback, but haven't heard much yet.

Original comment by [email protected] on 4 Aug 2011 at 12:04

• Added labels: Priority-Medium
• Removed labels: Priority-Low

[GoogleCodeExporter]

I've got feedback now, and there's been some pushback on adding in this 
functionality.  There's also been pushback on flag-categories, which would be a 
necessary partner to this API to solve the original problem.  So this is all on 
hold for the moment while I try to figure it all out.

Original comment by [email protected] on 26 Aug 2011 at 12:05

[GoogleCodeExporter]

I've been working on finding an API that stakeholders (within google) can agree 
to, but there's been a lot of pushback.  A general consensus is that the API 
should not be widened for this purpose (with a significant minority thinking it 
should not be widened for *any* purpose :-} ).

The suggestion instead is to just write a custom help-string (using 
SetUsageMessage), and use flag validators to make sure that only the correct 
flags are used with the correct commands.  This loses some of the advantages of 
automatic helpstring updating, but does let you organize the help in a more 
human-friendly way than the gflags default.

Based on that, I'm going to close this WillNotImplement.  Of course, you (and 
others who are interested) should feel free to patch in the above, or something 
like it, if that suits their needs better.  It just won't be part of the 
official distribution.

Original comment by [email protected] on 7 Oct 2011 at 10:41

• Changed state: WontFix