Closed Bug 1319225 Opened 8 years ago Closed 7 years ago

Support generated FINAL_TARGET_FILES in the tup backend

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mshal, Assigned: chmanchester)

References

Details

Attachments

(1 file)

Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.
59 bytes, text/x-review-board-request
mshal
: review+
Details 
We already handle non-generated FINAL_TARGET_FILES, but the generated ones require the relevant files to be built first. So we can add support for them as they get built.
Assignee: nobody → cmanchester
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review154176

This looks like it should be fine, though I'd like this to work on a .mozconfig with just ac_add_options --build-backends=Tup in it.

::: python/mozbuild/mozbuild/backend/tup.py:352
(Diff revision 1)
> -                    pass
> +                    # don't attempt to install files generated from them.
> +                    if f.context.relobjdir in ('layout/style/test',
> +                                               'toolkit/library'):
> +                        return
> +
> +                    output = mozpath.join('$(MOZ_OBJ_ROOT)', target, path,

Should we skip this for now if it's not an artifact build? Since we aren't compiling anything yet, a lot of these rules fail when I try to apply just this patch.
Attachment #8877345 - Flags: review?(mshal)
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review154176

> Should we skip this for now if it's not an artifact build? Since we aren't compiling anything yet, a lot of these rules fail when I try to apply just this patch.

I should have mentioned this, but it will fail without bug 1371817 applied. With that just `ac_add_options --build-backends=Tup` should work fine.
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review154534

::: python/mozbuild/mozbuild/backend/tup.py:350
(Diff revision 2)
>                  else:
> -                    # TODO: Support installing generated files
> -                    pass
> +                    # We're not generating files in these directories yet, so
> +                    # don't attempt to install files generated from them.
> +                    if f.context.relobjdir in ('layout/style/test',
> +                                               'toolkit/library'):
> +                        return

We don't actually want to return here, just skip those particular files. Otherwise we miss out on all the rest of the rules that we could generate during the obj.files.walk() loop.

::: python/mozbuild/mozbuild/backend/tup.py:355
(Diff revision 2)
> +                        return
> +
> +                    output = mozpath.join('$(MOZ_OBJ_ROOT)', target, path,
> +                                          f.target_basename)
> +                    gen_backend_file = self._get_backend_file(f.context.relobjdir)
> +                    gen_backend_file.symlink_rule(f.full_path, output=output,

This rule still doesn't work for me even with bug 1371817 applied :(. It tries to install a number of things we don't generate yet (.so files and the like). Tup also doesn't like us trying to install .xpt files that were generated from another directory, so those fail as well. (eg: js/xpconnect/tests/moz.build has '!/dist/bin/components/xpctest.xpt' in there). I'm fine with skipping those for now though since they're only apparently used by tests.
Attachment #8877345 - Flags: review?(mshal)
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review154534

> This rule still doesn't work for me even with bug 1371817 applied :(. It tries to install a number of things we don't generate yet (.so files and the like). Tup also doesn't like us trying to install .xpt files that were generated from another directory, so those fail as well. (eg: js/xpconnect/tests/moz.build has '!/dist/bin/components/xpctest.xpt' in there). I'm fine with skipping those for now though since they're only apparently used by tests.

Oh, this could be related to the bug you noticed above. Sorry about that!
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review154534

> Oh, this could be related to the bug you noticed above. Sorry about that!

Now that I'm looking in to this, there are a few more issues with test harness files, including with xpctest.xpt, and automation.py (which I just found is generated by a makefile).

If you skip "_tests" as the patch was before does it still fail? If so can you give some more examples of files it fails on?
Comment 8 is the version of the patch that ought to work, sorry about all the review spam.
(In reply to Chris Manchester (:chmanchester) from comment #7)
> Now that I'm looking in to this, there are a few more issues with test
> harness files, including with xpctest.xpt, and automation.py (which I just
> found is generated by a makefile).
> 
> If you skip "_tests" as the patch was before does it still fail? If so can
> you give some more examples of files it fails on?

Unfortunately yeah it still fails :(. I have bug 1371817 and the patch in #c8 applied, but it gets stuck in a few places. First, there's a bunch of files we don't generate yet since we aren't compiling things:

modules/libmar/tests
 - signmar

addon-sdk
 - addon-manager.xpi

uriloader/exthandler/tests
 - WriteArgument

toolkit/components/ctypes/tests
 - libjsctypes-test.so

toolkit/components/telemetry/tests
 - libmodules-test.so

Then there's the xpctest.xpt problem, which we do generate but I'm not sure yet how it should be handled in tup:

js/xpconnect/tests/idl
xpcom/tests
 - dist/bin/components/xpctest.xpt

(We can just postpone this one for later)

Then there's the automation.py generated from a Makefile, which affects two directories:

layout/tools/reftest
testing/mochitest
 - automation.py

Finally, I think the '**' not in f line is still required, otherwise I hit this:

tup error: Unable to create output file '**/setup.py' because it is already owned by command 427490.
 - [427490] _tests/mozbase/[!tup_ln /home/marf/mozilla-central-git/testing/mozbase/manifestparser/setup.py **/setup.py]

Is it working for you with just 'ac_add_options --build-backends=Tup'?
Hmm... almost all of these issues sound test related, I managed to upload the wrong version of the patch! Sorry again for the churn here.
Comment on attachment 8877345 [details]
Bug 1319225 - Install generated FinalTargetFiles in the Tup backend.

https://reviewboard.mozilla.org/r/148702/#review156442

Looks good - thanks for getting this to work!
Attachment #8877345 - Flags: review?(mshal) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66a13956eed8
Install generated FinalTargetFiles in the Tup backend. r=mshal
https://hg.mozilla.org/mozilla-central/rev/66a13956eed8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: