Closed Bug 1906736 Opened 4 months ago Closed 4 months ago

Regression in "make install" in embedder builds

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Version:
Firefox 128
Type:
defect

Tracking

(firefox-esr115 unaffected, firefox-esr128 fixed, firefox128 wontfix, firefox129 wontfix, firefox130 fixed)

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- fixed
firefox128 --- wontfix
firefox129 --- wontfix
firefox130 --- fixed

People

(Reporter: ptomato, Assigned: glandium)

References

(Regression)

Details

(Keywords: regression)

Attachments

(12 files)

Bug 1906736 - Fix INSTALL_NAME_TOOL invocation in standalone js `make install` after bug 1459764.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Make IMPORT_LIBRARY a full path.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Remove BaseRustLibrary.deps_path.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Make BaseRustLibrary.import_name a name, not a path.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Make Linkables' import_path an ObjDirPath.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Use RecursiveMakeBackend._pretty_path to output import_paths.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906736 - Remove BaseRustLibrary.deps_path.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review
Bug 1906736 - Make BaseRustLibrary.import_name a name, not a path.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review
Bug 1906736 - Make Linkables' import_path an ObjDirPath.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review
Bug 1906736 - Use RecursiveMakeBackend._pretty_path to output import_paths.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review
Bug 1906736 - Make IMPORT_LIBRARY a full path.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review
Bug 1906736 - Fix INSTALL_NAME_TOOL invocation in standalone js `make install` after bug 1459764.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review

This seems to be a regression from Bug 1459764. Running make install fails:

make[2]: *** No rule to make target 'libmozjs-128.so', needed by 'install'.  Stop.
make[2]: Leaving directory '/.../mozjs-128.0.0/js/src/_build/js/src/build'
make[1]: *** [Makefile:99: install] Error 2
make[1]: Leaving directory '/.../mozjs-128.0.0/js/src/_build/js/src'
make: *** [Makefile:225: install] Error 2

I believe this is due to an unforeseen interaction with the install rules in js/src/build/Makefile.in: https://searchfox.org/mozilla-central/source/js/src/build/Makefile.in#75-90

It looks like the SHARED_LIBRARY variable gained a full path, which is necessary because it is now generated in dist/bin. While the IMPORT_LIBRARY variable remained just a filename. (The import library is only generated on Windows. On other platforms, IMPORT_LIBRARY is supposed to be identical to SHARED_LIBRARY.)

The immediate cause of the failure is that Make looks for the install prerequisite IMPORT_LIBRARY (whose value is libmozjs-128.so) in the current directory $builddir/js/src/build/, whereas it should be located in $builddir/dist/bin/, and fails because there is no rule to build it in $builddir/js/src/build/.

However once that is fixed, we still need to ensure that IMPORT_LIBRARY and SHARED_LIBRARY have the same value on non-Windows platforms, to satisfy the ifneq ($(IMPORT_LIBRARY),$(SHARED_LIBRARY)) check below.

I think the easiest fix is probably in https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#1367, set IMPORT_LIBRARY to something like self._pretty_path(libdef.output_path, libdef.import_name) and I'm happy to make and test a patch for that, if it seems like the way to go.

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Keywords: regression
Regressed by: 1459764
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED

Can you test the attached patch?

Flags: needinfo?(philip.chimento)

This makes IMPORT_LIBRARY an absolute path, to match the change to
SHARED_LIBRARY so that the IMPORT_LIBRARY target can be found in
js/src/build/Makefile.in. Fixes regression in standalone build 'make
install'.

https://bugzilla.mozilla.org/show_bug.cgi?id=1906736

D215970 doesn't fix the immediate problem, which is that the prerequisites for the install target can't be satisfied.

I've attached a new patch that fixes the problem for me.

I think the first change of D215970 is correct and necessary as well, but the second change will no longer work if D215992 is accepted.

Flags: needinfo?(philip.chimento)

Set release status flags based on info from the regressing bug 1459764

status-firefox128: --- → affected
status-firefox129: --- → affected
status-firefox130: --- → affected
status-firefox-esr115: --- → unaffected
status-firefox-esr128: --- → affected

Its only use was removed in bug 1304815.

For consistency with other Library classes, import_name should be the
"import" library name, while import_path is the full path.

This will allow to use better backend primitives to use it in a
subsequent change.

Attachment #9411671 - Attachment description: Bug 1906736 - Make IMPORT_LIBRARY an absolute path. r?glandium → Bug 1906736 - Make IMPORT_LIBRARY a full path.
Attachment #9411628 - Attachment description: Bug 1906736 - Fix standalone js `make install` after bug 1459764. → Bug 1906736 - Fix INSTALL_NAME_TOOL invocation in standalone js `make install` after bug 1459764.
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/334aed124a54 Remove BaseRustLibrary.deps_path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/0ee858291b82 Make BaseRustLibrary.import_name a name, not a path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/cfcd1e782c34 Make Linkables' import_path an ObjDirPath. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/534d97cbb905 Use RecursiveMakeBackend._pretty_path to output import_paths. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/1b7f3d4ab195 Make IMPORT_LIBRARY a full path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/26b5993b476e Fix INSTALL_NAME_TOOL invocation in standalone js `make install` after bug 1459764. r=firefox-build-system-reviewers,nalexander

Backed out for causing python failure on test_recursivemake.py::TestRecursiveMakeBackend

Backout link

Push with failures

Failure log

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/a0bb59edb3c3 Remove BaseRustLibrary.deps_path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/682de9005b7f Make BaseRustLibrary.import_name a name, not a path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/743b41e65f84 Make Linkables' import_path an ObjDirPath. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/23022456c1af Use RecursiveMakeBackend._pretty_path to output import_paths. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/137ae2391cb1 Make IMPORT_LIBRARY a full path. r=firefox-build-system-reviewers,sergesanspaille https://hg.mozilla.org/integration/autoland/rev/9c1a7ace8507 Fix INSTALL_NAME_TOOL invocation in standalone js `make install` after bug 1459764. r=firefox-build-system-reviewers,nalexander

The patch landed in nightly and beta is affected.
:glandium, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox129 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(mh+mozilla)

Probably not important for Beta129, but we'll want this on ESR128. It does graft cleanly.

Yes, I want to get it on esr128, but bug 1907361 prevents that from happening at the moment.

status-firefox129: affected → wontfix
Attachment #9413161 - Flags: approval-mozilla-esr128?

For consistency with other Library classes, import_name should be the
"import" library name, while import_path is the full path.

Original Revision: https://phabricator.services.mozilla.com/D216141

Attachment #9413162 - Flags: approval-mozilla-esr128?

This will allow to use better backend primitives to use it in a
subsequent change.

Original Revision: https://phabricator.services.mozilla.com/D216142

Attachment #9413163 - Flags: approval-mozilla-esr128?
Attachment #9413164 - Flags: approval-mozilla-esr128?

This makes IMPORT_LIBRARY a full path, to match the change to
SHARED_LIBRARY so that the IMPORT_LIBRARY target can be found in
js/src/build/Makefile.in. Fixes regression in js standalone build 'make
install'.

Initial patch by Philip Chimento.

Original Revision: https://phabricator.services.mozilla.com/D215992

Attachment #9413165 - Flags: approval-mozilla-esr128?
Attachment #9413166 - Flags: approval-mozilla-esr128?

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no

esr128 Uplift Approval Request

  • User impact if declined: make install failure when building standalone JS
  • Code covered by automated testing: no
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: N/A
  • Risk associated with taking this patch: Low
  • Explanation of risk level: Build system only
  • String changes made/needed: N/A
  • Is Android affected?: no
Flags: needinfo?(mh+mozilla)
Attachment #9413161 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
Attachment #9413162 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
Attachment #9413163 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
Attachment #9413164 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
Attachment #9413165 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
Attachment #9413166 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: