869613 - make mach build some/deeper/path do dependencies for some and some/deeper

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joe Drew (not getting mail)]

Currently if I do mach build layout/base, the layout dependencies aren't done. They should be!

Comment 1

[Josh Matthews [:jdm]]

What dependencies are those?

Comment 2

[Joe Drew (not getting mail)]

toolkit/library

Comment 3

[Josh Matthews [:jdm]]

Ah, this is the problem being discussed on mozilla.dev.platform about longest matching substring vs. listing all subdirectories.

Comment 4

[Josh Matthews [:jdm]]

Relevant code to change is in http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/dumbmake/dumbmake.py.

Comment 5

[Tareq Khandaker]

Attachment: dumbmake.py.diff

My attempt at fixing this bug. I created a group of dummy folders to test my implementation.

mozilla-central
| zzz
|----- aaa
|----- bbb
        |--- ccc

My test was to execute ./mach build zzz/aaa zzz/bbb/ccc zzz/bbb

Without my changes, the output is:
 0:00.39 /usr/bin/make -j8 -s zzz/aaa
 0:00.52 /usr/bin/make -j8 -s zzz/bbb/ccc
 0:00.60 /usr/bin/make -j8 -s zzz/bbb
Your build was successful!

With my changes (see the attached diff), the output is:
 0:00.21 /usr/bin/make -j8 -s zzz
 0:00.26 /usr/bin/make -j8 -s zzz/aaa
 0:00.30 /usr/bin/make -j8 -s zzz/bbb
 0:00.34 /usr/bin/make -j8 -s zzz/bbb/ccc
Your build was successful!

Is this the desired output?

Thanks in advance

Comment 6

[Tareq Khandaker]

Correction: the folder layout is actually like this:

zzz
├── aaa
└── bbb
    └── ccc

3 directories, 0 files

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 804927 [details] [diff] [review]
dumbmake.py.diff

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

Thanks for the patch Tareq! This will give us correct results, with potentially some wasted work (since we will explicitly build subdirectories that presumably are built by the parent as well). If you'd like to work more on this, I'd like to try the following behaviour: for each component of a path, obtain the dependencies for that component, but do not add the component itself to the final target list. That means that if I run |./mach build layout/build|, the result should be |layout/build toolkit/library|, since layout/build has no dependencies but layout/ is a dependent of toolkit/library.

::: python/mozbuild/dumbmake/dumbmake.py
@@ +84,5 @@
>      for make_target, group in groupby(target_pairs, itemgetter(1)):
>          # Return non-simple directory targets untouched.
>          if make_target is not None:
>              for pair in group:
> +                paths = [pair[1]]

Add a comment like "Generate dependencies for all components of a path.
Given path a/b/c, examine a, a/b, and a/b/c."

Comment 8

[Tareq Khandaker]

Attachment: dumbmake.py.better.diff

The output for my test case (./mach build zzz/aaa zzz/bbb/ccc zzz/bbb) is the same as before: 

 0:00.21 /usr/bin/make -j8 -s zzz
 0:00.26 /usr/bin/make -j8 -s zzz/aaa
 0:00.30 /usr/bin/make -j8 -s zzz/bbb
 0:00.35 /usr/bin/make -j8 -s zzz/bbb/ccc
Your build was successful!

In addition, ./mach build layout/build now gives the following output: 

 0:00.21 /usr/bin/make -C toolkit/library -j8 -s
 0:00.39 /usr/bin/make -C layout/build -j8 -s
Your build was successful!

Comment 9

[Josh Matthews [:jdm]]

(In reply to Tareq Khandaker from comment #8)
> Created attachment 805090 [details] [diff] [review]
> dumbmake.py.better.diff
> 
> The output for my test case (./mach build zzz/aaa zzz/bbb/ccc zzz/bbb) is
> the same as before: 
> 
>  0:00.21 /usr/bin/make -j8 -s zzz
>  0:00.26 /usr/bin/make -j8 -s zzz/aaa
>  0:00.30 /usr/bin/make -j8 -s zzz/bbb
>  0:00.35 /usr/bin/make -j8 -s zzz/bbb/ccc
> Your build was successful!
> 
> In addition, ./mach build layout/build now gives the following output: 
> 
>  0:00.21 /usr/bin/make -C toolkit/library -j8 -s
>  0:00.39 /usr/bin/make -C layout/build -j8 -s
> Your build was successful!

That's an exciting result! The one problem is that layout/build needs to be built before toolkit/library.

Comment 10

[Josh Matthews [:jdm]]

Oh, and please use one of the review or feedback flags and give my name when uploading patches, otherwise it's easy to forget to review your work later.

Comment 11

[Tareq Khandaker]

Attachment: dumbmake.py.best.diff

I added a line to reverse the order, but I am not sure if that's the way to go for the general case.

./mach build zzz/aaa zzz/bbb/ccc zzz/bbb
 0:00.22 /usr/bin/make -j8 -s zzz
 0:00.27 /usr/bin/make -j8 -s zzz/aaa
 0:00.39 /usr/bin/make -j8 -s zzz/bbb
 0:00.44 /usr/bin/make -j8 -s zzz/bbb/ccc
Your build was successful!

./mach build layout/build
 0:00.76 /usr/bin/make -C layout/build -j8 -s
 0:00.93 /usr/bin/make -C toolkit/library -j8 -s
Your build was successful!

Comment 12

[Josh Matthews [:jdm]]

Sorry about the silence; I'll take a look at it today.

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 805733 [details] [diff] [review]
dumbmake.py.best.diff

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

I did look at it, and then I lost internet connectivity for a long time. This isn't going to work quite right (for example, if I write |mach build toolkit toolkit/library|), and I'll propose some changes that I think will fix that. What if we add a additional_dependencies function that takes a list of targets and returns the extra dependencies that those generate? With that, we should be able to return the list of explicit targets plus the list of extra dependencies - this should avoid us having to keep track of parents and remove them later, since they will never be added to the list.

Comment 14

[Tareq Khandaker]

What were you getting as the output for |mach build toolkit toolkit/library|? I got:

 0:00.23 /usr/bin/make -C browser/app -j8 -s
 0:00.77 /usr/bin/make -C toolkit/library -j8 -s

What should it be?

Thanks,

Comment 15

[Josh Matthews [:jdm]]

I would expect any directories explicitly listed on the commandline to be built, so toolkit/ would also be in that list. This is why I think we should avoid removing items from the list, and only add the dependencies.

Comment 16

[Tareq Khandaker]

Attachment: dumbmake.py.patch

I took another stab at it. Here's my output.

./mach build zzz/aaa zzz/bbb/ccc zzz/bbb
 0:00.24 /usr/bin/make -j8 -s zzz
 0:00.28 /usr/bin/make -j8 -s zzz/aaa
 0:00.32 /usr/bin/make -j8 -s zzz/bbb
 0:00.36 /usr/bin/make -j8 -s zzz/bbb/ccc

./mach build layout/build
 0:00.24 /usr/bin/make -C layout/build -j8 -s
 0:00.83 /usr/bin/make -C toolkit/library -j8 -s

/mach build toolkit toolkit/library
 0:00.24 /usr/bin/make -C toolkit -j8 -s
 0:00.63 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.65 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.66 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.72 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.82 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.08 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.16 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.16 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.17 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.19 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.20 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.23 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.39 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.60 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.65 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.65 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.71 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.73 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.73 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.77 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:02.06 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:02.47 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:06.74 /usr/bin/make -C browser/app -j8 -s
 0:07.28 /usr/bin/make -C toolkit/library -j8 -s

I simplified the code to have only one data structure for recording "visited" targets (visited) in add_extra_dependencies. I modified all_dependencies to not add the existing target to the list of dependencies (the function is only used in one place which is add_extra_dependencies).

Could you check if my understanding of how make_dirs are supposed to be processed is correct?
If you have |./mach build a a/b|, the output should be:
<a>
<dependencies of a>
<dependencies of a> (now for a/b)
<a/b>
<dependencies of a/b>

I think this set of changes satisfies the above ordering now.

Comment 17

[Josh Matthews [:jdm]]

This is looking better. However, what happens if you do |./mach build layout/build netwerk/build|? My fear is that we'll see a list like
>layout/build
>toolkit/library
>netwerk/build
which would be incorrect, since toolkit/library depends on both layout and netwerk.

Comment 18

[Tareq Khandaker]

Attachment: dumbmake.py.patch

Yeah that's the list that came out. Therefore, the make_dirs themselves (layout/build and netwerk/build) are the dependencies. Instead of going top-down like 'make', this process is bottom up.

Here's the output I have:

./mach build zzz/aaa zzz/bbb/ccc zzz/bbb
 0:00.23 /usr/bin/make -j8 -s zzz
 0:00.28 /usr/bin/make -j8 -s zzz/aaa
 0:00.32 /usr/bin/make -j8 -s zzz/bbb
 0:00.36 /usr/bin/make -j8 -s zzz/bbb/ccc

./mach build layout/build
 0:00.22 /usr/bin/make -C layout/build -j8 -s
 0:00.40 /usr/bin/make -C toolkit/library -j8 -s

The order changed for this test:
./mach build toolkit toolkit/library
 0:00.23 /usr/bin/make -C toolkit -j8 -s
 0:00.62 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.64 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.66 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.83 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.87 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.88 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.14 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.14 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.15 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.17 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.21 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.33 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.40 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.41 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.62 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.62 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.68 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.68 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.69 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.80 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:02.07 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:02.48 From ../../../dist/idl: Kept 1143 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:08.42 /usr/bin/make -C toolkit/library -j8 -s
 0:08.60 /usr/bin/make -C browser/app -j8 -s

And finally,
./mach build layout/build netwerk/build
 0:00.23 /usr/bin/make -C layout/build -j8 -s
 0:00.41 /usr/bin/make -C netwerk/build -j8 -s
 0:00.54 /usr/bin/make -C toolkit/library -j8 -s

Thanks,

Comment 19

[Tareq Khandaker]

I forgot to obsolete the patch from 2013-09-19 21:29 PDT.

Comment 20

[Mike Hommey [:glandium]]

With the now landed bug 907365 and bug 915648, and the upcoming bug 905973 (which will bring incremental builds in the order of seconds), I think we should kill WONTFIX this and kill dumbmake.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 807606 [details] [diff] [review]
dumbmake.py.patch

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

This looks fine to me; thanks for working on it! Could you upload a version following the steps at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F so that I can easily commit it?

Comment 22

[Tareq Khandaker]

Attachment: dumbmake.patch

I just did `hg qnew dumbmake.patch', and pasted in this bug's ID and name as the commit text.

Comment 23

[Tareq Khandaker]

Attachment: dumbmake.patch

Added r=jdm to the end of the commit message.

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 809587 [details] [diff] [review]
dumbmake.patch

Great; thanks!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d924e17bba4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for checktest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/583e9f08aeeb

https://tbpl.mozilla.org/php/getParsedLog.php?id=28347045&tree=Mozilla-Inbound

Comment 27

[Tareq Khandaker]

I didn't think about the fact that tests now have to be changed. I found that I could do |mach python-test python/mozbuild/dumbmake/test/test_dumbmake.py| to run them all, so I will edit test_dumbmake.py in order to get it to work.

Comment 28

[Tareq Khandaker]

Attachment: dumbmake.patch

I made a new patch.

./mach build zzz/aaa zzz/bbb/ccc zzz/bbb
 0:00.30 /usr/bin/make -j8 -s zzz
 0:00.49 /usr/bin/make -j8 -s zzz/aaa
 0:00.67 /usr/bin/make -j8 -s zzz/bbb
 0:00.85 /usr/bin/make -j8 -s zzz/bbb/ccc
Your build was successful!

./mach build layout/build
 0:00.30 /usr/bin/make -C layout/build -j8 -s
 0:01.53 /usr/bin/make -C toolkit/library -j8 -s
Your build was successful!

./mach build toolkit/library
 0:00.28 /usr/bin/make -C toolkit/library -j8 -s
 0:00.47 /usr/bin/make -C browser/app -j8 -s
Your build was successful!

./mach build layout/build netwerk/build
 0:00.29 /usr/bin/make -C layout/build -j8 -s
 0:00.63 /usr/bin/make -C netwerk/build -j8 -s
 0:00.86 /usr/bin/make -C toolkit/library -j8 -s
Your build was successful!

./mach build toolkit toolkit/library
 0:00.30 /usr/bin/make -C toolkit -j8 -s
 0:00.62 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.63 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.65 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.66 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.76 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.88 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.89 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.06 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.07 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.09 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.11 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.13 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.19 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.34 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.34 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.48 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.48 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.55 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.56 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.56 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.64 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.90 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:02.31 From ../../../dist/idl: Kept 1153 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:08.10 /usr/bin/make -C toolkit/library -j8 -s
 0:08.23 /usr/bin/make -C browser/app -j8 -s
Your build was successful!

./mach python-test python/mozbuild/dumbmake/test/test_dumbmake.py
 0:00.25 /home/user/src/mozilla-central/obj-ff-dbg/_virtualenv/bin/python python/mozbuild/dumbmake/test/test_dumbmake.py
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_add_extra_dependencies
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_all_dependencies
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_dependency_map
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_indentation
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_missing_entry
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_nested_dependencies
 0:00.30 TEST-PASS | python/mozbuild/dumbmake/test/test_dumbmake.py | test_two_dependencies

Comment 29

[Tareq Khandaker]

Is there anything left for me to do for this bug?

Thanks,

Comment 30

[Josh Matthews [:jdm]]

Comment on attachment 810308 [details] [diff] [review]
dumbmake.patch

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

Fix up the comment change, attach the patch, and feel free to set the checkin-needed keyword.

::: python/mozbuild/dumbmake/dumbmake.py
@@ +57,1 @@
>      """

Let's make sure to update the comment here to reflect the changes.

Comment 31

[Tareq Khandaker]

Attachment: dumbmake.patch

Comment 32

[Josh Matthews [:jdm]]

Note, the checkin flag is different from the checkin-needed keyword.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b17cbb36bb8

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b17cbb36bb8