923080 - Generate .xpt files directly into final location

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

We currently generate .xpt files into a staging directory then install them into $(FINAL_TARGET)/components during libs. We should just generate them directly into their final destination.

This is blocked on $(FINAL_TARGET) being available to moz.build files because we need to add the file references to the install manifest.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Generate xpt files into final location

Now that the mozbuild backend knows about FINAL_TARGET, we are able to
install generated xpt files into their final location. This saves us
from copying xpt files into their final location on every build.

The interaction with FINAL_TARGET is a bit hacky. But I think it works
OK.

I may or may not roll some interfaces.manifest work into this patch so
we can remove the silly buildlist.py rule out of rules.mk.

https://tbpl.mozilla.org/?tree=Try&rev=6ab5dca3a211

Comment 2

[Gregory Szorc [:gps]]

Reftests didn't like this patch :(

Comment 3

[Gregory Szorc [:gps]]

Attachment: Generate xpt files into final location

This fixes the issue with test_necko.xpt by adding an explicit
INSTALL_TARGET to the necko Makefile.in. Just another incremental hack
to support XPI_NAME. Did I mention how much I hate XPI_NAME?

https://tbpl.mozilla.org/?tree=Try&rev=ee9175eb2f6e

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 822494 [details] [diff] [review]
Generate xpt files into final location

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

::: Makefile.in
@@ +65,5 @@
>  js-config-status:
>  	$(call SUBMAKE,backend.RecursiveMakeBackend.built,js/src,1)
>  endif
>  
> +install_manifests := bin idl include public private sdk xpi-stage

xp*t*-stage. xpi is something else.

::: js/xpconnect/tests/idl/Makefile.in
@@ +5,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  componentdir = js/xpconnect/tests/components
> +libs:: $(FINAL_TARGET)/components/$(MODULE).xpt

Not really awesome to install test xpts but i guess we can't do much better for now.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +863,5 @@
> +        dist = mozpath.join(self.environment.topobjdir, 'dist')
> +        if path.startswith(dist):
> +            path = path[len(dist)+1:]
> +
> +        offset = path.index('/')

prefix, subpath = path.split('/', 1)

Note this fails if there is no '/' in the path, but on the other hand your code doesn't handle that well either. At least this version would fail with noise.

@@ +869,5 @@
> +
> +        key = 'dist_%s' % prefix
> +
> +        manifest = self._install_manifests[key]
> +        manifest.add_optional_exists(path[offset+1:])

manifest.add_optional_exists(subpath)

@@ +988,5 @@
> +            try:
> +                os.makedirs(d)
> +            except OSError as e:
> +                if e.errno != errno.EEXIST:
> +                    raise

I think that should be handled by dependencies on $(call mkdir_deps,...) instead.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +68,5 @@
>  
> +        # We strip 'dist' because most consumers don't care about that part.
> +        install_target = sandbox['FINAL_TARGET']
> +        offset = install_target.find('/')
> +        self.install_target = install_target[offset+1:]

Does FINAL_TARGET really always start with dist and nothing else significant? I'd feel better if you didn't strip this and used depth instead of dist when writing the xpt rules.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +95,5 @@
>          """
> +        # We only want to emit an InstallationTarget if one of the consulted
> +        # variables is defined. Later on, we look up FINAL_TARGET, which has
> +        # the side-effect of populating it. So, we need to do this lookup
> +        # early.

Shouldn't we have and use a sandbox.has/hasattr/contains/whatever, then ?

Comment 5

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

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 822494 [details] [diff] [review]
> Generate xpt files into final location
> 
> Review of attachment 822494 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Makefile.in
> @@ +65,5 @@
> >  js-config-status:
> >  	$(call SUBMAKE,backend.RecursiveMakeBackend.built,js/src,1)
> >  endif
> >  
> > +install_manifests := bin idl include public private sdk xpi-stage
> 
> xp*t*-stage. xpi is something else.

Actually, it is that one. We install WorkerTest.xpt into dist/xpi-stage/worker/components, so we need to track dist/xpi-stage.

Comment 6

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

Attachment: Generate xpt files into final location

Rebased

Comment 7

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

Attachment: Address comments

Note that this doesn't build yet.

Comment 8

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

In particular, the patches cause

IOError: [Errno 2] No such file or directory: 'js/src/_build_manifests/install/dist_xpi-stage'

and I'm way out of my depth to fix that.

Comment 9

[Brian O'Keefe [:bokeefe]]

https://tbpl.mozilla.org/?tree=Try&rev=ea85f8ae3dd3

Comment 10

[Brian O'Keefe [:bokeefe]]

I stumbled across this and thought I'd take a quick stab at updating the patches. The only major difference is that this part:

> ::: js/xpconnect/tests/idl/Makefile.in
> @@ +5,5 @@
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> >  componentdir = js/xpconnect/tests/components
> > +libs:: $(FINAL_TARGET)/components/$(MODULE).xpt
> 
> Not really awesome to install test xpts but i guess we can't do much better
> for now.

can be better now that TEST_HARNESS_FILES is a thing (although I did have to make that support absolute objdir paths).

Comment 11

[Brian O'Keefe [:bokeefe]]

Attachment: MozReview Request: bz://923080/bokeefe

Comment 12

[Brian O'Keefe [:bokeefe]]

/r/1187 - Bug 923080 - Generate xpt files into final location

Pull down this commit:

hg pull review -r ea85f8ae3dd3a27facb96e51569876fadb857b74

Comment 13

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8532821 [details]
MozReview Request: bz://923080/bokeefe

Looks like I broke xpcshell... I'll figure that out and re-request review.

Comment 14

[Brian O'Keefe [:bokeefe]]

/r/1187 - Bug 923080 - Generate xpt files into final location

Pull down this commit:

hg pull review -r d99f51d63c96fc7da7d8792fd739e6bf8668b892

Comment 15

[Brian O'Keefe [:bokeefe]]

(In reply to Brian O'Keefe [:bokeefe] from comment #13)
> Comment on attachment 8532821 [details]
> MozReview Request: bz://923080/bokeefe
> 
> Looks like I broke xpcshell... I'll figure that out and re-request review.

So, it turns out that js/xpconnect/tests/idl stopped being traversed during libs. I had to arrange for TEST_HARNESS_FILES to cause the libs tier not to be skipped when it emits the install targets for files in the objdir.

Green try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33926258bd16

Comment 16

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1185/#review715

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +        key = 'dist_%s' % prefix

This should probably fail if the path is not under dist.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +            path = path[len(dist)+1:]

use mozpath.relpath

::: netwerk/test/httpserver/Makefile.in
(Diff revision 2)
> +INSTALL_TARGETS += reftest_xpt

You should land this after bug 948278 and drop this hunk.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +                self._may_skip['libs'].remove(backend_file.relobjdir)

Ouch. This shouldn't be needed, and the reason why you need it is because TEST_HARNESS_FILES is not listed as a 'libs' variable in context.py. Change that there instead.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +        need_libs = False

You're not using this variable.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
>              'xpt/.mkdir.done'):

I think you can remove xpt/.mkdir.done here.

::: js/xpconnect/tests/idl/moz.build
(Diff revision 2)
> +TEST_HARNESS_FILES.xpcshell.js.xpconnect.tests.components.native += [

Please add a comment that this relies on xpctest.xpt ending up in dist/bin/components during export, and TEST_HARNESS_FILES being processed after that.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> -                '$(idl_xpt_dir)/%s.xpt: %s' % (module, ' '.join(idl_deps)),
> +            xpt_path = '%s/%s/components/%s.xpt' % (self.environment.topobjdir, install_target, module)

Use '$(DEPTH)' instead of self.environment.topobjdir?

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> -                '',
> +            rule.add_dependencies('$(dist_idl_dir)/%s.idl' % dep for dep in deps)

If we have all the idl dependencies, at this point we might as well point to the actual source instead of the $(dist_idl_dir) file, which we actually have no dependencies in place to ensure things are copied there before things happen here.

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +                '@echo "$(notdir $@)"',

$(notdir $@) can be written as $(@F)

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +        for d in set(os.path.dirname(p) for p in xpt_files):

I don't think this should be done here. Instead, each target xpt file should depend on mkdir_deps for their directory.

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 2)
> +        if context.get('FINAL_TARGET') or context.get('XPI_NAME') or \

any(k in context for k in ('FINAL_TARGET', ...))

::: config/makefiles/xpidl/Makefile.in
(Diff revision 2)
> -        $(dist_include_dir) $(idl_xpt_dir) $(idl_deps_dir)
> +        $(dist_include_dir) $(dir $@) $(idl_deps_dir)

$(dir $@) can be written as $(@D)

While here, you might as well make things smarter, by having a generic target here and only have dependencies emitted later.

something like:

%.xpt:
    @echo "$(@F)"
    $(PYTHON_PATH) $(PLY_INCLUDE) ... $(@D) $(idl_deps_dir) $(basename $(notdir $@ $(filter %.idl,$^)))

::: python/mozbuild/mozbuild/backend/recursivemake.py
(Diff revision 2)
> +from StringIO import StringIO

from io import StringIO (if that works)

Comment 17

[Brian O'Keefe [:bokeefe]]

/r/1187 - Bug 923080 - Generate xpt files into final location

Pull down this commit:

hg pull review -r e2dc0fe0adc225a1b8a9feb08c60ffcb024725e5

Comment 18

[Brian O'Keefe [:bokeefe]]

https://reviewboard.mozilla.org/r/1185/#review879

> You should land this after bug 948278 and drop this hunk.

I did this, and then wondered why I broke reftests in the try push. Obviously, not including the patch from that bug breaks it.

> from io import StringIO (if that works)

When I tried that, I got

0:37.71   File
"c:\mozilla-src\mozilla\python\mozbuild\mozbuild\makeutil.py", line 143,
in dump
 0:37.71     fh.write('\n')
 0:37.71 TypeError: unicode argument expected, got 'str'
 
so I just left it as-is.

> Ouch. This shouldn't be needed, and the reason why you need it is because TEST_HARNESS_FILES is not listed as a 'libs' variable in context.py. Change that there instead.

I was afraid doing that would cause all of the TEST_HARNESS_FILES directories to be recursed during libs, but it seems that's not the case. I compared before and after versions of root.mk, and they looked the same.

Comment 19

[Brian O'Keefe [:bokeefe]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02c72a723e9a

(Reftests are broken because this needs bug 948278 first)

Comment 20

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1185/#review999

::: python/mozbuild/mozbuild/frontend/context.py
(Diff revision 3)
> -        """, None),
> +        """, 'libs'),

Come to think of it, what you did in the previous iteration was actually theoretically better. BUT, most TEST_HARNESS_FILES directories have a Makefile.in anyways, so they'd be recursed anyways. I think it's fine for now. Please file a followup to make the objdir_files processing not use backend.mk.

Comment 21

[Brian O'Keefe [:bokeefe]]

(In reply to Mike Hommey [:glandium] from comment #20)
> https://reviewboard.mozilla.org/r/1185/#review999
> 
> ::: python/mozbuild/mozbuild/frontend/context.py
> (Diff revision 3)
> > -        """, None),
> > +        """, 'libs'),
> 
> Come to think of it, what you did in the previous iteration was actually
> theoretically better. BUT, most TEST_HARNESS_FILES directories have a
> Makefile.in anyways, so they'd be recursed anyways. I think it's fine for
> now. Please file a followup to make the objdir_files processing not use
> backend.mk.

Bug 1113622 covers fixing the objdir_files situation.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Brian O'Keefe [:bokeefe]]

Attachment: MozReview Request: Bug 923080 - Generate xpt files into final location