1474486 - cannot use comma-separated compiler flags as options value in mozconfig

Status: RESOLVED DUPLICATE of bug 1908637

(Firefox Build System :: General, defect)

Description

[u473386]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

Set this in mozconfig. It's a perfectly valid compiler flag value.

ac_add_options --enable-optimize="-fsanitize=address,fuzzer-no-link"

Run mach build.


Actual results:

 0:02.11 Traceback (most recent call last):
 0:02.11   File "/home/user/mozilla-central/configure.py", line 123, in <module>
 0:02.11     sys.exit(main(sys.argv))
 0:02.11   File "/home/user/mozilla-central/configure.py", line 29, in main
 0:02.11     sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/configure/__init__.py", line 409, in run
 0:02.11     self._value_for(option)
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/configure/__init__.py", line 477, in _value_for
 0:02.11     return self._value_for_option(obj)
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/util.py", line 944, in method_call
 0:02.11     cache[args] = self.func(instance, *args)
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/configure/__init__.py", line 520, in _value_for_option
 0:02.11     value, option_string = self._helper.handle(option)
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/configure/options.py", line 484, in handle
 0:02.11     ret = option.get_value(arg, origin)
 0:02.11   File "/home/user/mozilla-central/python/mozbuild/mozbuild/configure/options.py", line 359, in get_value
 0:02.11     self.nargs != 1) else ''
 0:02.11 mozbuild.configure.options.InvalidOptionError: --enable-optimize takes 0 or 1 values

Comment 1

[mparnell]

I can confirm this is an issue for me and has been for years, I work around it by just returning 1 in _validate_nargs for local builds...this isn't a valid fix for upstream but it at least lets you use the options you want to.

The issue comes down to the split_option() function in python/mozbuild/mozbuild/configure/options.py, where it creates a tuple from the result of splitting the argument by commas. It almost needs a way of respecting an escaped comma, and then remove the escapes after the fact or something.

Thoughts?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 4

[Adam Williamson]

This is now a problem for Fedora, because as of current Rawhide (what will become Fedora 39), we have a distro default definition of RUSTFLAGS with commas in it:

RUSTFLAGS='-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Cforce-frame-pointers=yes -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now --cap-lints=warn'

Trying to build Firefox with this RUSTFLAGS fails because of this exact problem:

mozbuild.configure.options.InvalidOptionError: RUSTFLAGS takes 1 value

I did try patching nargs to be "+" for RUSTFLAGS instead of "1", but that's not sufficient to fix it, because the split-on-commas that mparnell described means the commas actually get lost when the flags are passed to the compiler, so it gets garbage flags and fails:

0:29.67 error: failed to run rustc to learn about target-specific information
0:29.67 Caused by:
0:29.67 process didn't exit successfully: /usr/bin/rustc - --crate-name ___ --print=file-names -Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Cforce-frame-pointers=yes -Clink-arg=-Wl -z relro -Clink-arg=-Wl -z now --cap-lints=warn --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg (exit status: 1)
0:29.67 --- stderr
0:29.68 error: Unrecognized option: 'z'

I note that our CFLAGS and CXXFLAGS defaults also have commas in them, but it looks like those values aren't parsed through the same parser or already have special handling or something?

Comment 5

[Adam Williamson]

Attachment: patch that at least handles the RUSTFLAGS case

This patch seems to solve at least the RUSTFLAGS case. It's probably not really...enough, there are several cases where `split_option` is called where we wouldn't have an `Option` instance to use the `comma_split` value from; some of them aren't relevant, but one or two look like they'd probably be involved in the original case here (passing a cmdline arg). I'm not sure how to extend this to cover those offhand.

Another approach would be to use quoting; when the commas are within a quoted string, don't split on them. But I've no idea if the quoting "survives" down to the level of `split_option()` offhand, the code is way too complex to tell easily. Even if it does, I don't know what would be the best way of going about stuffing a layer of quote parsing in there.

Comment 6

[Alex Xu]

btw, I worked around this for RUSTFLAGS -Clink-arg by -Clink-arg=-z -Clink-arg=relro -Clink-arg=-z -Clink-arg=now or something like that.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Dupe of Bug 1908637 ?

Comment 8

[Adam Williamson]

Yes. Weird how that one got fixed in days while this one has been sitting around for six years. Oh well, at least it's fixed now.

Comment 9

[Daniel Veditz [:dveditz] out until Nov 10]

🤷🏽 It hadn't affected our continuous integration until now. Not weird at all that it suddenly got more visibility and got seen by the right people.