check_prog template should sanity check that `progs` is not a string

RESOLVED FIXED in Firefox 48

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

I was writing a patch that used `check_prog`, but I wrote:
```
yasm = check_prog('YASM', ('yasm'), allow_missing=True)
```

This didn't fail (because of allow_missing=True) but it didn't do what I expected because `('yasm') == 'yasm'`, so check_prog helpfully checked for ['y', 'a', 's', 'm'] in $PATH.

I have a patch to fix that, but it requires exposing 'isinstance' and 'tuple' in the ConfigureSandbox builtins. I've also added a `configure_error` template to use in other templates for situations like this where we want to report a programming error in the moz.configure files themselves.
Created attachment 8731714 [details]
MozReview Request: bug 1257542 - check_prog template should sanity check that progs is not a string. r?glandium

In Python parens around an expression without a trailing comma is not a tuple,
so ('foo') == 'foo'. This is really easy to screw up with check_progs, which
coerces progs to a list and would give you ['f','o','o'] in this case. This
patch enforces that the progs argument is a tuple or list and errors if it
is not.

Review commit: https://reviewboard.mozilla.org/r/40791/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40791/
Attachment #8731714 - Flags: review?(mh+mozilla)
Comment on attachment 8731714 [details]
MozReview Request: bug 1257542 - check_prog template should sanity check that progs is not a string. r?glandium

https://reviewboard.mozilla.org/r/40791/#review37481

::: build/moz.configure/util.configure:26
(Diff revision 1)
>      sys.exit(1)
>  
>  
>  @template
>  @advanced
> +def configure_error(message):

Please add a comment that this function is meant mainly for use in templates to throw errors about bad uses of them, not about "normal" configure errors.

::: python/mozbuild/mozbuild/configure/__init__.py:96
(Diff revision 1)
>  
>      # The default set of builtins.
>      BUILTINS = ReadOnlyDict({
>          b: __builtins__[b]
>          for b in ('None', 'False', 'True', 'int', 'bool', 'any', 'all', 'len',
> -                  'list', 'set', 'dict')
> +                  'list', 'set', 'dict', 'isinstance', 'tuple')

put tuple next to the other iterables.
Attachment #8731714 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5bd947546623f30a25113b0bb996d781ff938f2
bug 1257542 - check_prog template should sanity check that progs is not a string. r=glandium

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5bd94754662
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.