Closed Bug 1573566 Opened 7 months ago Closed 6 months ago

differentiate between "need for compilation" and "need for linking" in the compile tier

Categories

(Firefox Build System :: General, task)

task
Not set

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: froydnj, Assigned: glandium)

References

(Regressed 1 open bug)

Details

(Keywords: in-triage)

Attachments

(7 files)

I noticed today with a local build that js/src/jsapi-tests spent a lot of time waiting for JS's rust libs to be compiled (and, in turn, libjs.a). But that's somewhat silly: we should be able to compile the files in jsapi-tests without waiting for the rust library to build. The rust library should only block the linking of the jsapi-tests binary.

I guess this is kind of like bug 1278308?

Target split, kind of like bug 1572046?

(In reply to Mike Hommey [:glandium] from comment #1)

Target split, kind of like bug 1572046?

Almost, except that it needs to go a little bit deeper, IIUC. We currently drop PROGRAM in target, which then depends on all the objects. Moving PROGRAM to target-programs doesn't actually solve anything, because the objects (or their list file, depending) aren't driven off of being required for target, but instead off of being required for PROGRAM.

We need target-objects. Which would replace the changes in bug 1573314.

Assignee: nobody → mh+mozilla
Depends on: 1574058

The current setup, where gtest/libxul uses the static library in
the same directory as the shared libxul, and somehow the backend ignores
gkrust for gtest/libxul, is fragile.

The existing check is no thorough enough: it only checks that multiple
Rust libraries are not directly linked to the same binary, but that's
not enough. In fact, until the change prior to this one, this was
happening to xul-gtest, and without that change, this is what configure
would now have to say:

Cannot link the following Rust libraries into the "xul-gtest" library:
- gkrust-gtest
- gkrust
Only one is allowed.

This only ended up not being a problem because the backend somehow works
around the problem, which it shouldn't have to do.

So that isinstance(foo, StaticLibrary) doesn't end up being true for
HostRustLibrary. Instead, it now inherits from HostLibrary.

Apart from the need to link them last, they don't actually need to be
treated any different from normal static libraries.

HostLibrary is always an expand lib. HostRustLibrary is always a
non-expand lib. But we currently rely on type checking to figure that
out, rather than an attribute, like for StaticLibrary. Doing that
simplifies existing and upcoming code.

With the addition of toolkit/library/build because of the rust
shenanigans, bug 1573314 and bug 1572046 don't do anything useful
anymore. We're going to do something better anyways.

Make the target and host targets depend on those, and flatten the
dependencies.

That is, instead of chains like:
browser/app/target: mozglue/build/target
mozglue/build/target: memory/build/target

we now have:
browser/app/target: browser/app/target-objects
mozglue/build/target-objects
memory/build/target-objects

Which means Make can feel free to build the object files in browser/app
before building other dependencies.

Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/14329e051f16
Move the real libxul definition in a subdirectory. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/295a5e78ed1e
Fully check that two Rust libraries aren't being linked to the same binary. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e4285304b231
Don't make HostRustLibrary indirectly inherit from StaticLibrary. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d55d48534b6f
Make rust libraries more like normal libraries. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/83033ba2c21f
Make host libraries more like static libraries. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/8afcc2c2c508
Undo bug 1573314 and bug 1572046. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c51ba76790c0
Add target/host-objects targets to build objects. r=froydnj
Regressions: 1579156
Regressions: 1579502
Regressions: 1578530
Regressions: 1584038
You need to log in before you can comment on or make changes to this bug.