1259960 - Make check_prog() more flexible

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259960 - s/DummyFunction/DependsFunction/. r?chmanchester

While DummyFunction is descriptive of what the instances are (and they
can't even be called), the various uses of isintance(obj, DummyFunction)
are kind of confusing, especially when they are in moz.configure land
(and this bug is about to add another one).

Review commit: https://reviewboard.mozilla.org/r/42629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42629/

Comment 2

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259960 - Allow to pass a string to check_prog to describe what is being looked for. r?chmanchester

So far, we've been using the lowercase of the variable name, but it's
not enough for some special cases. Those special cases could do their
own business, but then, they'd have to duplicate 90% of check_prog,
which is less desirable.

Review commit: https://reviewboard.mozilla.org/r/42631/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42631/

Comment 3

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259960 - Make check_prog more flexible about the input it receives. r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/42633/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42633/

Comment 4

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259960 - Make check_prog more flexible about the list of programs it will check. r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/42635/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42635/

Comment 5

[Mike Hommey [:glandium]]

Notes for the reviewer:
- this needs bug 1257516 applied first.
- the use-cases for the extra flexibility are actually in the unit test.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8735176 [details]
MozReview Request: Bug 1259960 - s/DummyFunction/DependsFunction/. r?chmanchester

https://reviewboard.mozilla.org/r/42629/#review39229

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8735177 [details]
MozReview Request: Bug 1259960 - Allow to pass a string to check_prog to describe what is being looked for. r?chmanchester

https://reviewboard.mozilla.org/r/42631/#review39231

::: build/moz.configure/checks.configure:81
(Diff revision 1)
>  # will look for 'a' or 'b' in $PATH, and set_config PROG to the one
>  # it can find. If PROG is already set from the environment or command line,

This part of the comment isn't grammatical anymore. "This will look for..."

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8735178 [details]
MozReview Request: Bug 1259960 - Make check_prog more flexible about the input it receives. r?chmanchester

https://reviewboard.mozilla.org/r/42633/#review39235

::: build/moz.configure/checks.configure:107
(Diff revision 1)
> -    option(env=var, nargs=1,
> +        option(env=var, nargs=1,
> -           help='Path to %s' % (what or 'the %s program' % var.lower()))
> +               help='Path to %s' % (what or 'the %s program' % var.lower()))

This becomes awkward, because specifying `input` changes the meaning of `var` a lot: if someone does `check_prog('PROG', ('a', 'b'), input='--with-prog')`, they might be surprised they don't get the ability to set this from the environment for free.

If we consider check_prog a mostly build-system internal thing this is fine, but it should be called out in the comment for check_prog.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/42633/#review39235

> This becomes awkward, because specifying `input` changes the meaning of `var` a lot: if someone does `check_prog('PROG', ('a', 'b'), input='--with-prog')`, they might be surprised they don't get the ability to set this from the environment for free.
> 
> If we consider check_prog a mostly build-system internal thing this is fine, but it should be called out in the comment for check_prog.

Oh, I see it is mentioned in the comment. Nevermind.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8735179 [details]
MozReview Request: Bug 1259960 - Make check_prog more flexible about the list of programs it will check. r?chmanchester

https://reviewboard.mozilla.org/r/42635/#review39239

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebd549a0af9
https://hg.mozilla.org/integration/mozilla-inbound/rev/273794fe2b2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/58ea10aa8c89
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c01ff2b685a

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7ebd549a0af9
https://hg.mozilla.org/mozilla-central/rev/273794fe2b2c
https://hg.mozilla.org/mozilla-central/rev/58ea10aa8c89
https://hg.mozilla.org/mozilla-central/rev/7c01ff2b685a