If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"TypeError: cannot compare a unicode against an NegativeOptionValue; OptionValue instances are tuples - did you mean to compare against member elements using [x]?" bustage on Windows since bug 1375231 landed

RESOLVED FIXED in Firefox 56

Status

()

Core
Build Config
--
critical
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: RyanVM, Assigned: gps)

Tracking

({regression})

Trunk
mozilla56
Unspecified
Windows
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
As reported by dvander in bug 1375231, I too am currently broken by bug 1375231 in the same manner.
(Reporter)

Comment 1

3 months ago
With the logging line requested by glandium:

 0:15.56 checking for Windows SDK... 0x0a00 in 'C:\Program Files (x86)\Windows Kits\10\'
 0:15.56 checking for Universal CRT SDK... u'C:\\Program Files (x86)\\Windows Kits\\8.1\\' NegativeOptionValue()
 0:15.60 Traceback (most recent call last):
 0:15.60   File "c:/Users/Ryan/repos/mozilla/configure.py", line 124, in <module>
 0:15.60     sys.exit(main(sys.argv))
 0:15.60   File "c:/Users/Ryan/repos/mozilla/configure.py", line 29, in main
 0:15.60     sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 426, in run
 0:15.60     func(*args)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 472, in _value_for
 0:15.60     return self._value_for_depends(obj, need_help_dependency)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\util.py", line 925, in method_call
 0:15.60     cache[args] = self.func(instance, *args)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 481, in _value_for_depends
 0:15.60     return obj.result(need_help_dependency)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\util.py", line 925, in method_call
 0:15.60     cache[args] = self.func(instance, *args)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 122, in result
 0:15.60     return self._func(*resolved_args)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 1000, in wrapped
 0:15.60     return new_func(*args, **kwargs)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 733, in wrapper
 0:15.60     ret = template(*args, **kwargs)
 0:15.60   File "c:/Users/Ryan/repos/mozilla/build/moz.configure/checks.configure", line 53, in wrapped
 0:15.60     ret = func(*args, **kwargs)
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 1000, in wrapped
 0:15.60     return new_func(*args, **kwargs)
 0:15.60   File "c:/Users/Ryan/repos/mozilla/build/moz.configure/windows.configure", line 190, in valid_ucrt_sdk_dir
 0:15.60     if d == windows_sdk_dir_env:
 0:15.60   File "c:\Users\Ryan\repos\mozilla\python\mozbuild\mozbuild\configure\options.py", line 64, in __eq__
 0:15.60     type(other).__name__, type(self).__name__))
 0:15.60 TypeError: cannot compare a unicode against an NegativeOptionValue; OptionValue instances are tuples - did you mean to compare against member elements using [x]?
I see what's going on... we have:

    if windows_sdk_dir_env:
        windows_sdk_dir_env = windows_sdk_dir_env[0]

and since windows_sdk_dir_env is a NegativeOptionValue, it keeps its value. We tend to have such patterns here and there, and I'm afraid we're going to hit similar breakage kind of randomly :-/

gps, how about relaxing the check added in bug 1375231 to not apply to empty OptionValues?
Flags: needinfo?(gps)
(In reply to Mike Hommey [:glandium] from comment #2)
> I see what's going on... we have:
> 
>     if windows_sdk_dir_env:
>         windows_sdk_dir_env = windows_sdk_dir_env[0]
> 
> and since windows_sdk_dir_env is a NegativeOptionValue, it keeps its value.
> We tend to have such patterns here and there, and I'm afraid we're going to
> hit similar breakage kind of randomly :-/

like, you could very well have hit this on line 123 instead of 189. I'm sure there are other places where something similar can happen.
(Assignee)

Comment 4

3 months ago
I'm fine relaxing the check to not apply to empty OptionValues.

(I'm kinda surprised not one of our automation configs triggered this.)
Flags: needinfo?(gps)
So the workaround here is to backout https://hg.mozilla.org/mozilla-central/rev/51a92a22d6d1

Could we backout this before we have some solution to this bug? I bet it would hit many Windows developers when they update their local tree.
Yeah, I had to backout this locally to make the tree buildable.
(Assignee)

Comment 7

3 months ago
I'll work on a patch right now.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
A lot of build peers are away today, it appears. If anyone who claims to know Python wants to do a drive-by review so this can get landed, please do.

Comment 10

3 months ago
mozreview-review
Comment on attachment 8884935 [details]
Bug 1379298 - Relax __eq__ for empty OptionValue;

https://reviewboard.mozilla.org/r/155768/#review160866

lgtm.

::: python/mozbuild/mozbuild/test/configure/test_options.py:306
(Diff revision 1)
> +        # easily compare value-less types like PositiveOptionValue and
> +        # NegativeOptionValue.
> +        empty_positive = PositiveOptionValue()
> +        empty_negative = NegativeOptionValue()
> +        self.assertEqual(empty_positive, ())
> +        self.assertEqual(empty_negative, ())

Maybe this is clear, or maybe these tests already exists, but can you assert what happens when you compare to `True` and `False` as well?  I found that surprising more than once.
Attachment #8884935 - Flags: review+

Comment 11

3 months ago
mozreview-review
Comment on attachment 8884935 [details]
Bug 1379298 - Relax __eq__ for empty OptionValue;

https://reviewboard.mozilla.org/r/155768/#review160872

LGTM although nalexander's suggestion is worthwhile.

::: python/mozbuild/mozbuild/test/configure/test_options.py:306
(Diff revision 1)
> +        # easily compare value-less types like PositiveOptionValue and
> +        # NegativeOptionValue.
> +        empty_positive = PositiveOptionValue()
> +        empty_negative = NegativeOptionValue()
> +        self.assertEqual(empty_positive, ())
> +        self.assertEqual(empty_negative, ())

Not a bad idea. Something like the `self.assertTrue(empty_positive)` test we have in other sections/
Attachment #8884935 - Flags: review?(giles) → review+
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
Comment on attachment 8884935 [details]
Bug 1379298 - Relax __eq__ for empty OptionValue;

https://reviewboard.mozilla.org/r/155768/#review160866

> Maybe this is clear, or maybe these tests already exists, but can you assert what happens when you compare to `True` and `False` as well?  I found that surprising more than once.

Comparing explicitly against `True` and `False` is a bit touchy because boolean compares should be handled by `__nonzero__` instead of `__eq__`. But in some cases, yes, ``__eq__`` can get invoked.

Since this bug is blocking people from building on Windows, I'd rather punt additional work to a follow-up.
Comment hidden (mozreview-request)

Comment 14

3 months ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b8398ced330
Relax __eq__ for empty OptionValue; r=nalexander,rillian

Comment 15

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b8398ced330
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.