Closed Bug 950298 Opened 6 years ago Closed 6 years ago

Make the js build system use top-level as its root objdir

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

First baby step in the direction of bug 950297.

configure will still leave in js/src, but running it will create a tree with at least mfbt, js/src and intl/icu in it.
Depends on: 950291, 950290, 950027
Depends on: 951010
This passed try on an older tree, except for the Hf job. I'll attach incremental patches for fixups.
Attachment #8357709 - Flags: review?(gps)
Comment on attachment 8357709 [details] [diff] [review]
Make the js build system use top-level as its root objdir

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

::: moz.build
@@ +21,3 @@
>  
> +    if CONFIG['ENABLE_CLANG_PLUGIN']:
> +      add_tier_dir('base', 'build/clang-plugin', external=True)

Two more spaces while you're reindenting
Depends on: 958295
This is the hg rm part.
Attachment #8358090 - Flags: review?(gps)
Same as the previous, without the hg rm parts, and with a few fixes. It now doesn't break hazard analysis builds on try because bug 958295 has landed.
Attachment #8358218 - Flags: review?(gps)
Attachment #8357709 - Attachment is obsolete: true
Attachment #8357709 - Flags: review?(gps)
With indentation fixed as per ms2ger's comment.
Attachment #8358220 - Flags: review?(gps)
Attachment #8358218 - Attachment is obsolete: true
Attachment #8358218 - Flags: review?(gps)
Blocks: 958404
Comment on attachment 8358090 [details] [diff] [review]
Remove js/src/config and js/src/build

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

/me pours gasoline and lights a match.
Attachment #8358090 - Flags: review?(gps) → review+
Comment on attachment 8358220 [details] [diff] [review]
Make the js build system use top-level as its root objdir

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

Crazy patch. It looks sane to me. Super excited about what this is setting us up for!

I think you should do a PGO try before landing if you haven't already - if history has taught me anything, it's than any changes to /Makefile.in end up breaking PGO on first landing.

::: Makefile.in
@@ +126,5 @@
> +# finishing to merge gecko and js build systems, removing the need for the
> +# latter.
> +ifdef BUILDING_JS
> +NO_REMOVE=1
> +endif

There's a followup bug lingering here.

::: config/rules.mk
@@ +1461,5 @@
>  
>  # If we're using binary nsinstall and it's not built yet, fallback to python nsinstall.
> +ifneq (,$(filter $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX),$(install_cmd)))
> +ifeq (,$(wildcard $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX)))
> +nsinstall_is_usable = $(if $(wildcard $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX)),yes)

Why are you using $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX) instead of $(NSINSTALL)?

Can we just move this check to config.mk? Should we just build nsinstall more specially than we do today? Although, uses of nsinstall will wane with everything moving to mozpack.

Followup bugs, I reckon.

::: config/static-checking-config.mk
@@ +47,4 @@
>  CLANG_PLUGIN := $(DEPTH)/build/clang-plugin/$(DLL_PREFIX)clang-plugin$(DLL_SUFFIX)
> +else
> +CLANG_PLUGIN := $(DEPTH)/../../build/clang-plugin/$(DLL_PREFIX)clang-plugin$(DLL_SUFFIX)
> +endif

Shouldn't CLANG_PLUGIN be defined in config.mk?

::: moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  CONFIGURE_SUBST_FILES += [
> +    'config/autoconf.mk',
> +    'config/emptyvars.mk',

Ah, I was wondering where these went!

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +552,5 @@
>                  if dirs:
>                      # For build systems without tiers (js/src), output a list
>                      # of directories for each tier.
> +                    all_dirs = list(self._traversal.traverse('', filter))
> +                    root_mk.add_statement('%s_dirs := %s' % (tier, ' '.join(all_dirs)))

You don't need to cast to a list since .join() will take a generator.

::: testing/testsuite-targets.mk
@@ +478,5 @@
>  stage-xpcshell: make-stage-dir
>  	$(MAKE) -C $(DEPTH)/testing/xpcshell stage-package
>  
>  stage-jstests: make-stage-dir
> +	$(MAKE) -C $(DEPTH)/js/src/js/src/tests stage-package

That's a funky path!
Attachment #8358220 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 8358220 [details] [diff] [review]
> Make the js build system use top-level as its root objdir
> 
> Review of attachment 8358220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Crazy patch. It looks sane to me. Super excited about what this is setting
> us up for!
> 
> I think you should do a PGO try before landing if you haven't already - if
> history has taught me anything, it's than any changes to /Makefile.in end up
> breaking PGO on first landing.
> 
> ::: Makefile.in
> @@ +126,5 @@
> > +# finishing to merge gecko and js build systems, removing the need for the
> > +# latter.
> > +ifdef BUILDING_JS
> > +NO_REMOVE=1
> > +endif
> 
> There's a followup bug lingering here.

?

> ::: config/rules.mk
> @@ +1461,5 @@
> >  
> >  # If we're using binary nsinstall and it's not built yet, fallback to python nsinstall.
> > +ifneq (,$(filter $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX),$(install_cmd)))
> > +ifeq (,$(wildcard $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX)))
> > +nsinstall_is_usable = $(if $(wildcard $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX)),yes)
> 
> Why are you using $(DIST)/bin/nsinstall$(HOST_BIN_SUFFIX) instead of
> $(NSINSTALL)?

Because NSINSTALL is either nsinstall or nsinstall.py, and we obviously don't want this block to work for nsinstall.py. Note I'm not changing any logic here.
 
> ::: config/static-checking-config.mk
> @@ +47,4 @@
> >  CLANG_PLUGIN := $(DEPTH)/build/clang-plugin/$(DLL_PREFIX)clang-plugin$(DLL_SUFFIX)
> > +else
> > +CLANG_PLUGIN := $(DEPTH)/../../build/clang-plugin/$(DLL_PREFIX)clang-plugin$(DLL_SUFFIX)
> > +endif
> 
> Shouldn't CLANG_PLUGIN be defined in config.mk?

This *is* config.mk (indirectly).
 
> ::: testing/testsuite-targets.mk
> @@ +478,5 @@
> >  stage-xpcshell: make-stage-dir
> >  	$(MAKE) -C $(DEPTH)/testing/xpcshell stage-package
> >  
> >  stage-jstests: make-stage-dir
> > +	$(MAKE) -C $(DEPTH)/js/src/js/src/tests stage-package
> 
> That's a funky path!

Temporary until things are actually merged.
https://hg.mozilla.org/mozilla-central/rev/1a1968da61b3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
See Also: → 959933
Depends on: 959519
Depends on: 969801
Blocks: 1403928
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.