Closed Bug 1043802 Opened 5 years ago Closed 5 years ago

Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attachment #8462339 - Flags: review?(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?
Attachment #8462339 - Flags: review?(gps) → review+
I figured I could remove a few more.
Attachment #8462343 - Flags: review?(gps)
Attachment #8462339 - Attachment is obsolete: true
(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 on attachment 8462343 [details] [diff] [review]
Remove TOOLS_DIRS and TEST_TOOLS_DIRS

Let me address the first review comments.
Attachment #8462343 - Flags: review?(gps)
Attachment #8462346 - Flags: review?(gps)
Attachment #8462343 - Attachment is obsolete: true
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.
Attachment #8462358 - Flags: review?(gps)
Attachment #8462346 - Attachment is obsolete: true
Attachment #8462346 - Flags: review?(gps)
Blocks: 1043820
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 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.
Attachment #8462358 - Flags: review?(gps) → review+
I'm not planning to add more.
https://hg.mozilla.org/mozilla-central/rev/ac2f9ea38a56
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.