1052526 - mach build-docs broken from OS_LIBS refactoring

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

|mach build-docs| is busted.

mach build-docs evaluates each moz.build file (not just the active moz.build files) in isolation by calling reader.walk_topsrcdir(). The assumption that CONFIG['EDITLINE_LIBS'] is defined is not valid in this execution mode.

----------

Error running mach:

    ['build-docs']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

BuildReaderError:
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /Users/gps/src/firefox/js/src/shell/moz.build

The error was triggered on line 33 of this file:

    OS_LIBS += CONFIG['EDITLINE_LIBS']

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['ValueError: Only lists can be appended to lists.\n']


  File "/Users/gps/src/firefox/tools/docs/mach_commands.py", line 49, in build_docs
    for sandbox in reader.walk_topsrcdir():
  File "/Users/gps/src/firefox/python/mozbuild/mozbuild/frontend/reader.py", line 712, in walk_topsrcdir
    filesystem_absolute=True, read_tiers=True):
  File "/Users/gps/src/firefox/python/mozbuild/mozbuild/frontend/reader.py", line 758, in read_mozbuild
    sys.exc_info()[2], sandbox_exec_error=se)

Comment 1

[Gregory Szorc [:gps]]

Attachment: Conditionally append EDITLINE_LIBS

|mach build-docs| failed to parse this moz.build file in isolation
because EDITLINE_LIBS is not always defined. This patch fixes that and
unbreaks |mach build-docs|.

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8471650 [details] [diff] [review]
Conditionally append EDITLINE_LIBS

Can we solve this more generally by allowing '+= (None)' to do nothing? Ie: something like:

    def __iadd__(self, other):
        if other is None:
            return self
        ...

If you'd rather not go that route, this patch looks fine. I think we'd probably hit similar bugs in the future, though.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8471650 [details] [diff] [review]
Conditionally append EDITLINE_LIBS

Review of attachment 8471650 [details] [diff] [review]:
-----------------------------------------------------------------

EDITLINE_LIBS is an AC_SUBST_LIST. When it's empty, its value is []. There must be something else wrong... like configure being outdated.

Comment 4

[Gregory Szorc [:gps]]

EDITLINE_LIBS is not defined in my config.status and not present in my configure script. I don't see what conditional its AC_SUBST_LIST is behind. Something seems very weird.

Comment 5

[Michael Shal [:mshal]]

It should be in obj/js/src/config.status, but I guess that's not sourced?

Comment 6

[Gregory Szorc [:gps]]

Gah. Looking at wrong config.status. From js/src/config.status:

(''' EDITLINE_LIBS ''', list(r'''  '''.split())),

Comment 7

[Gregory Szorc [:gps]]

I think this is a bug in reader.walk_topsrcdir() not applying the proper config.status or descending into directories belonging to external build systems. This bug was likely uncovered by OS_LIBS foo.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #7)
> I think this is a bug in reader.walk_topsrcdir() not applying the proper
> config.status or descending into directories belonging to external build
> systems. This bug was likely uncovered by OS_LIBS foo.

The OS_LIBS += CONFIG['EDITLINE_LIBS'] is exercised on m-c builds with a [] value already. It has to be something else.

Comment 9

[Mike Hommey [:glandium]]

Ah, I see what you mean.

Comment 10

[Mike Hommey [:glandium]]

... and I might have something related in my patch queue.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #10)
> ... and I might have something related in my patch queue.

but it doesn't help.

So the problem is that walk_topsrcdir() doesn't follow DIRS, and doesn't pass down config environments when going through subdirectories. So js/src/moz.build does get js/src/config.status environment, but not the subdirectories.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Treat appending None to List as empty list

As a special exception, we change List's + and += operators to treat
"+ None" and "+= None" as "+ []" and "+= []" respectively.

This is a hack to make moz.build files simpler so they don't have to
perform "is x" checks before appending x.

While I was here, I fixed the implementation of List.__add__ to return a
List instead of list.

This hack solves the undefined EDITLINE_LIBS nicely. The more proper
solution is to fix walk_topsrcdir(). But that is more work because it
requires moz.build to know which files we shouldn't descend into. I'd
rather not hack up traversal right now because it's already in a "fun"
state.

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/239fef1233e5

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/239fef1233e5