Closed Bug 1375231 Opened 3 years ago Closed 3 years ago

Fix OptionValue value comparisons

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(3 files)

As discovered in bug 1374824, stylo_config() and webrender() were comparing literal string values against OptionValue moz.configure types and this would never be true because __eq__ does a type check and the proper thing to do is compare value[0] instead of the OptionValue itself.

We need to at least fix webrender(). We may also want to add better checking to OptionValue.__eq__ to prevent this class of bugs.

Since I got bit by this today, I'll produce a patch.
I don't see --enable-webrender=build in any mozconfigs, I /think/ we're not accidentally shipping WebRender. Whew.
Comment on attachment 8880118 [details]
Bug 1375231 - Make OptionValue.__eq__ more type aware;

https://reviewboard.mozilla.org/r/151508/#review156458

::: python/mozbuild/mozbuild/configure/options.py:60
(Diff revision 1)
>  
>      def __eq__(self, other):
> +        # This is to catch naive comparisons against strings and other
> +        # types in moz.configure files, as it is really easy to write
> +        # value == 'foo'.
> +        if not isinstance(other, OptionValue):

You should allow tuples. Instantiating OptionValues in moz.configure is not really meant to be a thing.
Comment on attachment 8880118 [details]
Bug 1375231 - Make OptionValue.__eq__ more type aware;

https://reviewboard.mozilla.org/r/151508/#review156458

> You should allow tuples. Instantiating OptionValues in moz.configure is not really meant to be a thing.

And yes, I think __eq__ shouldn't return false when other is a tuple, which it currently does with the type() check. I don't know why I wrote it that way.
Comment on attachment 8880117 [details]
Bug 1375231 - Properly compare value for --enable-webrender;

https://reviewboard.mozilla.org/r/151506/#review157818
Attachment #8880117 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8880118 [details]
Bug 1375231 - Make OptionValue.__eq__ more type aware;

https://reviewboard.mozilla.org/r/151508/#review157836

::: python/mozbuild/mozbuild/configure/options.py:64
(Diff revision 2)
> +        # value == 'foo'.
> +        if not isinstance(other, tuple):
> +            raise TypeError('cannot compare a %s against an %s; OptionValue '
> +                            'instances are tuples - did you mean to compare '
> +                            'against member elements using [x]?' % (
> +                                type(other), type(self)))

%s formatting for type(foo) is "<type 'Type'>", maybe type(foo).__name__ would be better.
Attachment #8880118 - Flags: review?(mh+mozilla) → review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4749e0a2e63
Properly compare value for --enable-webrender; r=glandium
https://hg.mozilla.org/integration/autoland/rev/51a92a22d6d1
Make OptionValue.__eq__ more type aware; r=glandium
https://hg.mozilla.org/mozilla-central/rev/e4749e0a2e63
https://hg.mozilla.org/mozilla-central/rev/51a92a22d6d1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attached file backtrace.txt
This broke my Windows build, I can't get past the configure step. Backtrace attached. Changing the exception to a "return False" lets me build again.
(In reply to David Anderson [:dvander] from comment #14)
> Created attachment 8884357 [details]
> backtrace.txt
> 
> This broke my Windows build, I can't get past the configure step. Backtrace
> attached. Changing the exception to a "return False" lets me build again.

Can you add:
  log.info('%r %r', d, windows_sdk_dir_env)

above the if d == windows_sdk_dir_env on build/moz.configure/windows.configure line 189 and report what that says?
(also, please file a new bug)
Flags: needinfo?(dvander)
Depends on: 1379298
I filed bug 1379298.
Flags: needinfo?(dvander)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.