Closed Bug 869613 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: joe, Assigned: tareqakhandaker)

References

(Depends on 1 open bug)

Details

(Whiteboard: [mentor=jdm][lang=py])

Attachments

(1 file, 8 obsolete files)

Currently if I do mach build layout/base, the layout dependencies aren't done. They should be!
What dependencies are those?
toolkit/library
Ah, this is the problem being discussed on mozilla.dev.platform about longest matching substring vs. listing all subdirectories.
Whiteboard: [mentor=jdm][lang=py]
Attached patch dumbmake.py.diff (obsolete) — Splinter Review
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
Correction: the folder layout is actually like this:

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

3 directories, 0 files
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."
Attachment #804927 - Flags: feedback+
Attached patch dumbmake.py.better.diff (obsolete) — Splinter Review
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!
Attachment #804927 - Attachment is obsolete: true
(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.
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.
Component: mach → Build Config
Attached patch dumbmake.py.best.diff (obsolete) — Splinter Review
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!
Attachment #805090 - Attachment is obsolete: true
Attachment #805733 - Flags: feedback?(josh)
Attachment #805733 - Flags: feedback?(josh) → review?(josh)
Sorry about the silence; I'll take a look at it today.
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.
Attachment #805733 - Flags: review?(josh)
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,
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.
Attached patch dumbmake.py.patch (obsolete) — Splinter Review
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.
Attachment #805733 - Attachment is obsolete: true
Attachment #807584 - Flags: feedback?(josh)
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.
Attached patch dumbmake.py.patch (obsolete) — Splinter Review
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,
Attachment #807606 - Flags: feedback?(josh)
Attachment #807584 - Flags: feedback?(josh)
I forgot to obsolete the patch from 2013-09-19 21:29 PDT.
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 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?
Attachment #807606 - Flags: feedback?(josh) → review+
Attachment #807584 - Attachment is obsolete: true
Attached patch dumbmake.patch (obsolete) — Splinter Review
I just did `hg qnew dumbmake.patch', and pasted in this bug's ID and name as the commit text.
Attachment #807606 - Attachment is obsolete: true
Attachment #809582 - Flags: review?(josh)
Attached patch dumbmake.patch (obsolete) — Splinter Review
Added r=jdm to the end of the commit message.
Attachment #809582 - Attachment is obsolete: true
Attachment #809582 - Flags: review?(josh)
Attachment #809587 - Flags: review?(josh)
Comment on attachment 809587 [details] [diff] [review]
dumbmake.patch

Great; thanks!
Attachment #809587 - Flags: review?(josh) → review+
Keywords: checkin-needed
Assignee: nobody → tareqakhandaker
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.
Attached patch dumbmake.patch (obsolete) — Splinter Review
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
Attachment #809587 - Attachment is obsolete: true
Attachment #810308 - Flags: review?(josh)
Is there anything left for me to do for this bug?

Thanks,
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.
Attachment #810308 - Flags: review?(josh) → review+
Attached patch dumbmake.patchSplinter Review
Attachment #810308 - Attachment is obsolete: true
Attachment #811163 - Flags: checkin+
Attachment #811163 - Flags: checkin+
Note, the checkin flag is different from the checkin-needed keyword.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b17cbb36bb8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 924676
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.