1043802 - Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

With bug 1043344 building things in the right order, we don't need the TOOL_DIRS/TEST_TOOL_DIRS clauses to have things built after libxul.

Sadly, we do still have things that do require the tools target, but they are few enough that we can just detect them in Makefile.in.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8462339 [details] [diff] [review]
Remove TOOLS_DIRS and TEST_TOOLS_DIRS

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

::: config/recurse.mk
@@ +105,2 @@
>  # At least build/export requires config/export for buildid, but who knows what
>  # else, so keep this global dependency to make config/export first for now.

The comment is now a lie. I assume you tracked down the unknown dependencies outside of the export tier?

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +733,5 @@
> +                    for t in (b'XPI_PKGNAME', b'INSTALL_EXTENSION_ID',
> +                            b'tools'):
> +                        if t not in content:
> +                            continue
> +                        if t == 'tools' and not re.search('(?:^|\s)tools.*::', content, re.M):

Nit: there's an implicit cast between b'tools' and 'tools'

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +598,5 @@
>              s.write('\n')
> +
> +            if inner.args[2] in DEPRECATION_HINTS:
> +                s.write('%s\n' % DEPRECATION_HINTS[inner.args[2]])
> +                return

Nice.

@@ +606,5 @@
> +            s.write('\n')
> +            s.write('The variable %s causing the error is:\n' % verb)
> +            s.write('\n')
> +            s.write('    %s\n' % inner.args[2])
> +            s.write('\n')

Is Splinter or you inserting the redundant block of output here?

Comment 3

[Mike Hommey [:glandium]]

Attachment: Remove TOOLS_DIRS and TEST_TOOLS_DIRS

I figured I could remove a few more.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8462339 [details] [diff] [review]
> Remove TOOLS_DIRS and TEST_TOOLS_DIRS
> 
> Review of attachment 8462339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/recurse.mk
> @@ +105,2 @@
> >  # At least build/export requires config/export for buildid, but who knows what
> >  # else, so keep this global dependency to make config/export first for now.
> 
> The comment is now a lie. I assume you tracked down the unknown dependencies
> outside of the export tier?

It's actually not. The ifeq was misplaced in bug 1043344 and that made all things depend on config/$tier, not only config/export for the export tier rules.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8462343 [details] [diff] [review]
Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Let me address the first review comments.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Comment 7

[Mike Hommey [:glandium]]

Attachment: Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Had a MemoryError on android because of a explosion of nodes in the directory traversal for the tools tiers, because of a bug in handling parallel dirs when there are very few directories. But since there are so few in that tier anyways, and none of them under PARALLEL_DIRS, let's just completely serialize that tier.

Which makes the distinction between PARALLEL_DIRS and DIRS useless since the directory traversal filters are now either putting everything in the parallel bucket, or in the sequential bucket. Will file a new bug for that.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8462358 [details] [diff] [review]
Remove TOOLS_DIRS and TEST_TOOLS_DIRS

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

::: js/xpconnect/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DIRS += ['public', 'idl', 'wrappers', 'loader', 'src']
> +DIRS += ['shell']

Nit: throw this into the list above

::: security/manager/ssl/tests/moz.build
@@ +10,5 @@
>      'gtest',
>      'mochitest',
>  ]
>  
> +TEST_DIRS += [

Ditto

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8462358 [details] [diff] [review]
Remove TOOLS_DIRS and TEST_TOOLS_DIRS

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +734,5 @@
> +                            b'tools'):
> +                        if t not in content:
> +                            continue
> +                        if t == b'tools' and not re.search('(?:^|\s)tools.*::', content, re.M):
> +                            continue

So hacky. I kinda wish we could use the pymake API for this. But yeah, this is easiest.

Is it worth moving the "is variable in" or "is target in" checks into standalone functions? Not sure how much you are going to use these in the near future.

Comment 10

[Mike Hommey [:glandium]]

I'm not planning to add more.

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2f9ea38a56

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac2f9ea38a56