Nathan Froyd [:froydnj] Reporter
	Description • 7 years ago

As seen in https://treeherder.mozilla.org/#/jobs?repo=try&revision=501d42aa00cdbc7a320c74885f81bb155cec3ab4

08:42:22     INFO -  error: Could not compile `cssparser`.
08:42:22     INFO -  Caused by:
08:42:22     INFO -    process didn't exit successfully: `/builds/slave/try-m64-0000000000000000000000/build/src/rustc/bin/rustc --crate-name cssparser /builds/slave/try-m64-0000000000000000000000/build/src/third_party/rust/cssparser/src/lib.rs --crate-type lib --emit=dep-info,link -C opt-level=2 -C panic=abort -C debuginfo=2 -C metadata=a01061401b84b9f7 -C extra-filename=-a01061401b84b9f7 --out-dir /builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/release/deps --target x86_64-apple-darwin -C linker=/builds/slave/try-m64-0000000000000000000000/build/src/build/cargo-linker -L dependency=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/release/deps -L dependency=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/release/deps --extern procedural_masquerade=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/release/deps/libprocedural_masquerade-ef777dde05f78242.rlib --extern phf=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/release/deps/libphf-9f797902d69cb573.rlib --extern matches=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/release/deps/libmatches-f5f54fdb7565d683.rlib --extern cssparser_macros=/builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/toolkit/library/release/deps/libcssparser_macros-64a596fb045f2854.dylib --cap-lints allow` (signal: 5, SIGTRAP: trace/breakpoint trap)
08:42:22     INFO -  Build failed, waiting for other jobs to finish...

Ralph, do you know of any fixes to rustc along these lines?

	Ralph Giles (:rillian) Assignee
	Comment 1 • 7 years ago

Sorry, no, I've never done a mac stylo build in automation. That's not much of an error message, but I've started a local build to see if I can reproduce it, but it's worked in the recent past. I notice from the log that sccache keeps falling over, but I think it's not enable for rust yet?

	Ralph Giles (:rillian) Assignee
	Comment 2 • 7 years ago

The only thing I can think of which is different on try is that we're using cargo 1.19.0-beta.1 to get RUSTC_WRAPPER support, along with rustc 1.17.0-stable. rustc 1.19.0-beta.2 can't compile gecko-bindings (bug 1365300) but it's possible just cargo has some macos-specific problem. I notice the tc debug mac-cross build in the same push succeeded.

	Nathan Froyd [:froydnj] Reporter
	Comment 3 • 7 years ago

Can we downgrade the cargo we're using there?  We could default to disabling Stylo on our Mac builds, but I don't know that I want to go that far...

	Ralph Giles (:rillian) Assignee
	Comment 4 • 7 years ago

I'd rather upgrade if possible, so as not to block Ted's work. I'll do a try push.

	Ralph Giles (:rillian) Assignee
	Comment 5 • 7 years ago

It looks like cargo 1.19.0-beta.2 is the same as 1.19.0-beta.1 which we're currently using. The build still fails, but with stdc++ issues, so maybe inbound wasn't clean. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c10bc751a0ba0673a917c7dfb8b29834eaa7e19&selectedJob=100275396

I hit a packaging change trying cargo nightly. I'm out of time for today but will try to push cargo-nightly or a reversion to cargo stable when I can.

	Ralph Giles (:rillian) Assignee
	Comment 6 • 7 years ago

cargo 1.20-nightly fails the same way. I also rebased back onto m-c in case other inbound check-ins where causing issues. Note that I still get the stdc++ issues on the opt build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c27e2689ddc0c3816d7c67247cf38a99d24b0ef9&selectedJob=100285183

Maybe it's another macOS 10.7 compatibility issue.

	Chris Peterson [:cpeterson]
	Comment 7 • 7 years ago

Ralph, since you've started to look into this Mac problem, can I assign this bug to you? It is one of our blocking bugs for building Stylo in Nightly by default(but preffed off).

	Ralph Giles (:rillian) Assignee
	Comment 8 • 7 years ago

Sure, although I only have one idea left at this point.

	Ralph Giles (:rillian) Assignee
	Comment 9 • 7 years ago

I've tried various combinations of old and new cargo (and older rustc) without success. I've requested a loaner to see if I can figure out what's going on manually.

	Chris Peterson [:cpeterson]
	Comment 10 • 7 years ago

Manish or Emilio, do you have any suggestions for what might be causing these cargo errors on the Mac build machines?

	Emilio Cobos Álvarez (:emilio)
	Comment 11 • 7 years ago

I have no clue, sorry :(

	Manish Goregaokar [:manishearth]
	Comment 12 • 7 years ago

No idea. SIGTRAP sounds bad. Maybe Alex knows?

	Alex Crichton [:acrichto]
	Comment 13 • 7 years ago

Sorry I always have a lot of trouble reading the logs here, I can't even find where the gisted logs in comment #1 came from :(. Some questions I'd have:

* Can someone help me look at the logs? Where does "Caused by:" get printed?
* Are we able to capture a stack trace at error time here?
* The OSX builders are using 10.7, right?

I can't think of much offhand that will generate SIGTRAP except maybe like a serious compiler bug, but I don't know of any that have cropped up recently or been fixed recently. In any case I'll try to help keep digging into this!

	Ralph Giles (:rillian) Assignee
	Comment 14 • 7 years ago

Thanks Alex. The logs are from the link in comment #1. Click on one of the red 'B' status symbols in the macOS 10.7 opt line, then click on one of the `log...` links in the Failure Classification tab at the bottom.

I have a loaner and can reproduce. 10.7 isn't supported, of course, so almost nothing works, but I checked out gecko-dev with the git on the system, put /usr/local/bin first in PATH so configure detects python 2.7.3 there, ran `./mach artifact toolchain --tooltool-manifest browser/config/tooltool-manifests/macosx64/releng.manifest` to download clang, installed rustup and used it to install rust 1.17.0. Then I was able to start a firefox build.
unicode-bidi v0.3.1

Three crates fail consistently: cssparser v0.13.5, unicode-bidi v0.3.1, webrender_traits v0.39.0.

I think the 'error: Could not compile' message comes from the cargo, based on grepping for the error in the source (no hits in firefox, some in cargo). But no other information is printed.

Running cargo just in a stand-alone crate directory does fail with errors, but I think they're different from what we're seeing with Firefox. Anything with dependencies fails fetching the registry:

> dyld: lazy symbol binding failed: Symbol not found: _regcomp_l
>   Referenced from: /Users/cltbld/.rustup/toolchains/stable-x86_64-apple-darwin/bin/cargo
>   Expected in: /usr/lib/libSystem.B.dylib

A crate with no deps compiles but cannot link because the system linker isn't compatible in the usual way (Apple llvm-gcc 4.2.1).

>  Assertion failed: (_mode == modeFinalAddress), function finalAddress, file /SourceCache/ld64/ld64-123.2.1/src/ld/ld.hpp, line 573.

If I set `CARGO_TARGET_X86_64_APPLE_DARWIN_LINKER` to clang from the toolchain download we use to build firefox, cargo build, run, and test all work, so it's specific to these crates. They're hard to test independently without being able to download the deps. Perhaps I can hack a Cargo.lock to use Firefox's vendored source.

Running the rustc invocation in the debugger, I was able to get a partial backtrace:

> #0  0x00007fff8e78b649 in exit ()
> #1  0x0000000100233af4 in std::process::exit::h658c32fb4db0741f ()
> #2  0x000000010003b6fe in rustup_init::run_multirust::hd7fd21473f804155 ()
> #3  0x000000010003851a in rustup_init::main::h862036313af1ae91 ()
> #4  0x000000010023ebdb in __rust_maybe_catch_panic ()
> #5  0x000000010023dc87 in std::rt::lang_start::h74b3afbdd8daef1c ()
> #6  0x0000000100001674 in start ()

No line numbers though, so perhaps not very helpful. If you have any suggestions how to proceed from here, please let me know.

	Ralph Giles (:rillian) Assignee
	Comment 15 • 7 years ago

Greg, Coop, what's the status of upgrading the m-c builders to a supported macOS release? I thought after bug 1338651 came to an impasse we decided to go ahead with upgrades, but I can't find a corresponding releng bug. It sure would be nice to stop having to support these machines.

	Greg Arndt [:garndt]
	Comment 16 • 7 years ago

Amy, in light of the cross compiled builds not working, do we have a plan for upgrading these OS X machines until another suitable solution is found?  I imagine we're locked into the version while we're on older hardware.

	Ralph Giles (:rillian) Assignee
	Comment 17 • 7 years ago

(In reply to Greg Arndt [:garndt] from comment #16)
 
> I imagine we're locked into the version while we're on older hardware.

The loaner I have is a Macmini5,3 (wikipedia says 'mid 2011'). Apple's hardware requirements for sierra (10.12, the current release) are "mid 2010 and later" for Mac minis, so hardware isn't a constraint unless that machine is unusual.

	Amy Rich [:arr] [:arich]
	Comment 18 • 7 years ago

I have not heard a suggestion that we might do OS upgrades from any of the groups involved in managing the builds. 

My understanding was that we were waiting till at least the end of 2017 to see if the cross compiled build problem could be solved. The systems that we do have are old (2012), failing, and long out of warranty. We have some spares (old test machines of the same generation that we retired last year) that we're currently using to swap in when hardware dies. I'm not sure we have the capacity to do rolling upgrades even if we wanted to, but I'll leave it to coop or catlee to weigh in on that. We certainly don't want to carry these machines over to a new datacenter.

Since the plan was to migrate to cross compiled builds in AWS, there was no budget allocated to replace these machines in 2017, so unless there's an approval for an out of budget spend of $175K - $200K, the earliest we'd see new hardware is probably mid Q1 2018. We'd also need unbudgeted engineer time to set up the deployment and management automation for a new platform.

To help me understand the needs, how much build capacity do we require for this? Would this be the entire pool? A few machines?

	Alex Crichton [:acrichto]
	Comment 19 • 7 years ago

Thanks for the info Ralph! Turns out I needed to scroll *down* on the logs, not up to find the error... 

In any case so this looks very interesting actually. The error coming out of Cargo means the child process exited abnormally, and that child process exited with SIGTRAP. I don't recall anything off-hand that generates SIGTRAP in LLVM/Rust's libstd by default, but I'm likely forgetting something.

The backtrace you got looks like it may actually be of rustup, not rustc, though. It can be difficult to debug these tools with the rustup shims in place :(. Now that *may* mean a SIGTRAP is coming from rustup? But I imagine on automation there's no rustup it's just raw rustc, right?

As for the other errors, I have no idea what's going on there either :(. If Cargo can't load that's quite surprising because we intend Cargo to be loaded on 10.7 and up, and if the linker is hitting assertions that's... unfortunate! For me to help more here I think it'd be best to try to get a backtrace of the process that's dying from SIGTRAP, that may help to illuminate a lot here as to what's happening.

Can you also confirm the `rustc -vV` of the rustc that's generating the SIGTRAP? (just to have the info on hand)

	Chris Cooper [:coop] (he/him)
	Comment 20 • 7 years ago

(In reply to Amy Rich [:arr] [:arich] from comment #18)
> Since the plan was to migrate to cross compiled builds in AWS, there was no
> budget allocated to replace these machines in 2017, so unless there's an
> approval for an out of budget spend of $175K - $200K, the earliest we'd see
> new hardware is probably mid Q1 2018. We'd also need unbudgeted engineer
> time to set up the deployment and management automation for a new platform.

I still believe cross-compilation is the realistic path forward for Mac.

Where are we with the debugging the performance problems? Are there more things we could try, or are we stuck? Is there value in a multi-group team spending a day at the all-hands to dig into this? 

If we're going to meet at the all-hands, I could pull in mshal and ted, but we'd also need (at least) wcosta and jmaher to help.

	Greg Arndt [:garndt]
	Comment 21 • 7 years ago

> I still believe cross-compilation is the realistic path forward for Mac.
> 
> Where are we with the debugging the performance problems? Are there more
> things we could try, or are we stuck? Is there value in a multi-group team
> spending a day at the all-hands to dig into this? 
> 
> If we're going to meet at the all-hands, I could pull in mshal and ted, but
> we'd also need (at least) wcosta and jmaher to help.

I definitely agree that cross compiled builds should be our path forward.  

The latest updates are in this bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1338651

Unfortunately in some cases we've regressed more than 30%, which is significant.  Also, since this might be at the IPC layer, some of the work we're doing this year could actually result in even more of a regression there.

Wander will be finishing up some remaining OS X test work and then revisiting the cross compiled builds.  He tried to rebuild it yesterday to get some more details for others, but the build failed.  He needs to spend some time investigating.

The more expertise we can get on this the better. I think having multiple people talk and work on this is a great idea, but can we afford to wait until the All Hands to do that?

	Ralph Giles (:rillian) Assignee
	Comment 22 • 7 years ago

(In reply to Alex Crichton [:acrichto] from comment #19)

> The backtrace you got looks like it may actually be of rustup, not rustc,
> though.

Oh, of course! Yes, you're right, the automation builds don't use the rustup ship.

Here's a backtrace from the rustc the automation build would use, recreating cargo's invocation when building the unicode-bidi crate.

Starting program: /Users/cltbld/gecko-dev/rustc/bin/rustc -vV
rustc 1.17.0 (56124baa9 2017-04-24)
binary: rustc
commit-hash: 56124baa9e73f28c0709e59e74783cf234a978cf
commit-date: 2017-04-24
host: x86_64-apple-darwin
release: 1.17.0
LLVM version: 3.9

Starting program: /Users/cltbld/gecko-dev/rustc/bin/rustc --crate-name unicode_bidi /Users/cltbld/gecko-dev/third_party/rust/unicode-bidi/src/lib.rs --cfg 'feature="serde"' --cfg 'feature="with_serde"' --cfg 'feature="serde_derive"' -C metadata=8a3f04c6725af877 -C extra-filename=-8a3f04c6725af877 --out-dir /Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/x86_64-apple-darwin/release/deps --target x86_64-apple-darwin -C linker=/Users/cltbld/gecko-dev/build/cargo-linker -L dependency=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/x86_64-apple-darwin/release/deps -L dependency=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/release/deps --extern serde_derive=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/release/deps/libserde_derive-45b50952e63a7467.dylib --extern matches=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/x86_64-apple-darwin/release/deps/libmatches-5d718a941939cd5a.rlib --extern serde=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/x86_64-apple-darwin/release/deps/libserde-938e5fb20e3c6ac9.rlib --extern serde_test=/Users/cltbld/gecko-dev/obj-x86_64-apple-darwin11.2.0/toolkit/library/x86_64-apple-darwin/release/deps/libserde_test-f82303c4b25f6007.rlib
[...]
Program received signal SIGTRAP, Trace/breakpoint trap.
(gdb) bt
#0  0x00007fff8b71252f in __CFInitialize ()
#1  0x00007fff5fc0fde3 in __dyld__ZN16ImageLoaderMachO11doImageInitERKN11ImageLoader11LinkContextE ()
#2  0x00007fff5fc0fa5b in __dyld__ZN16ImageLoaderMachO16doInitializationERKN11ImageLoader11LinkContextE ()
#3  0x00007fff5fc0d258 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#4  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#5  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#6  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#7  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#8  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#9  0x00007fff5fc0d1f1 in __dyld__ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEjRNS_21InitializerTimingListE ()
#10 0x00007fff5fc0e02b in __dyld__ZN11ImageLoader15runInitializersERKNS_11LinkContextERNS_21InitializerTimingListE ()
#11 0x00007fff5fc03189 in __dyld__ZN4dyld15runInitializersEP11ImageLoader ()
#12 0x00007fff5fc095cb in __dyld_dlopen ()
#13 0x00007fff9061595b in dlopen ()
#14 0x000000010175cc78 in rustc_back::dynamic_lib::DynamicLibrary::open::h71807c19786b1602 ()
#15 0x0000000100c69f5e in rustc_metadata::creader::CrateLoader::register_crate::h22eebf3c33390537 ()
#16 0x0000000100c6c91c in rustc_metadata::creader::CrateLoader::resolve_crate::h20596488ec0c7c09 ()
#17 0x0000000100c73282 in _$LT$rustc_metadata..creader..CrateLoader$LT$$u27$a$GT$$u20$as$u20$rustc..middle..cstore..CrateLoader$GT$::process_item::hedf3ddb3c9d0ee58 ()
#18 0x0000000100e98e0d in rustc_resolve::build_reduced_graph::_$LT$impl$u20$rustc_resolve..Resolver$LT$$u27$a$GT$$GT$::build_reduced_graph_for_item::h50023b8268967f74 ()
#19 0x0000000100e9f77b in _$LT$rustc_resolve..build_reduced_graph..BuildReducedGraphVisitor$LT$$u27$a$C$$u20$$u27$b$GT$$u20$as$u20$syntax..visit..Visitor$LT$$u27$a$GT$$GT$::visit_item::hf40374a6b53a6c96 ()
#20 0x0000000100e9f9af in _$LT$rustc_resolve..build_reduced_graph..BuildReducedGraphVisitor$LT$$u27$a$C$$u20$$u27$b$GT$$u20$as$u20$syntax..visit..Visitor$LT$$u27$a$GT$$GT$::visit_item::hf40374a6b53a6c96 ()
#21 0x0000000100e9180f in rustc_resolve::macros::_$LT$impl$u20$syntax..ext..base..Resolver$u20$for$u20$rustc_resolve..Resolver$LT$$u27$a$GT$$GT$::visit_expansion::h71ae8a22348e6c0f ()
#22 0x0000000103c8d240 in syntax::ext::expand::MacroExpander::collect_invocations::hb41ec1de218eded1 ()
#23 0x0000000103c89df2 in syntax::ext::expand::MacroExpander::expand::hf7d89690970df208 ()
#24 0x0000000103c89914 in syntax::ext::expand::MacroExpander::expand_crate::h9f11367fa410f86f ()
#25 0x00000001000a4c64 in rustc_driver::driver::phase_2_configure_and_expand::_$u7b$$u7b$closure$u7d$$u7d$::h83f82b5438ae7fdd ()
#26 0x000000010009ee95 in rustc_driver::driver::phase_2_configure_and_expand::h7993466b5ba9f43d ()
#27 0x0000000100098735 in rustc_driver::driver::compile_input::h4a3867b4383a9600 ()
#28 0x00000001000dc87d in rustc_driver::run_compiler::hb11017c6b45be0d6 ()
#29 0x000000010000e509 in std::panicking::try::do_call::hc2d47b7090d8b7b0 ()
#30 0x000000010420903b in __rust_maybe_catch_panic ()
#31 0x000000010002a884 in _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::hddb88924f2a81c7b ()
#32 0x0000000104202c65 in std::sys::imp::thread::Thread::new::thread_start::h4008e1859fbd98b8 ()
#33 0x00007fff8e7988bf in _pthread_start ()
#34 0x00007fff8e79bb75 in thread_start ()

	Ralph Giles (:rillian) Assignee
	Comment 23 • 7 years ago

We may be hitting https://openradar.appspot.com/7209349 a bug which wasn't reproducible in macOS 10.9. The crash is in loading derived macros dylib from a worker thread.

	Alex Crichton [:acrichto]
	Comment 24 • 7 years ago

Ok I've debugged live a bit with Ralph and in addition to the bug mentioned above:

* Procedural macros for Rust are compiled as dynamic libraries
* One of the crates in the OP, cssparser_macros, is a procedural macro
* All x86_64-apple-darwin linker invocations use a custom wrapper script in Gecko
* This custom linker script injects linkage to libraries like Cocoa/objc
* When dynamically loading cssparser_macros this runs dynamic initialization functions
* When loading cssparser_macros this means CFInitialize is called
* Due to https://openradar.appspot.com/7209349 this causes a SIGTRAP

The rustc compiler currently runs the whole compilation on a non-main thread to have more control over the thread's stack size (it gives it a huge stack), so that's where the non-main-thread is introduced. It looks like the SIGTRAP bug was at least fixed in 10.9, so this bug seems isolated to 10.7/10.8 at best?

Some possible fixes include:

* Add the ability for rustc to run the compilation on the main thread (accepting that the stack may be blown)
* Avoid linking Cocoa/objc to procedural macros (somehow, not sure the best way to do this)

Unfortunately I'm not sure what the best way to solve this is :(

	Nathan Froyd [:froydnj] Reporter
	Comment 25 • 7 years ago

(In reply to Alex Crichton [:acrichto] from comment #24)
> * All x86_64-apple-darwin linker invocations use a custom wrapper script in
> Gecko
> * This custom linker script injects linkage to libraries like Cocoa/objc

We could try to strip out the custom linkage in the script itself?

	Ralph Giles (:rillian) Assignee
	Comment 27 • 7 years ago

I got a successful build stripping the frameworks and -lobjc from the LDFLAGS passed to cargo. While this patch is brittle, I think it's simpler since this is supposed to be a temporary issue and it should unblock stylo for this platform.

Pushed to try with stylo enabled to confirm and check for side effects on other platforms. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a92424ea8e26bb0da4230f66bacc9467d68e2b1a

I'm open to the idea in #24 if you feel strongly, though. Might have to make it a python script. :)

	Ralph Giles (:rillian) Assignee
	Comment 28 • 7 years ago

Hmm, I tried to push just this patchto review board, the the try push there seems to have pulled in the stylo enable patch as an ancestor anyway, like the push in #27. Seems to reduce the problem to the libstdc++ mismatch issue, anyway.

	Ralph Giles (:rillian) Assignee
	Comment 29 • 7 years ago

Does no harm, anyway. Here's a green try push without the stylo enable.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37eec966637d7afc661672b1bf58849ae3f357fd

	Ralph Giles (:rillian) Assignee
	Comment 30 • 7 years ago

Re patching rustc, Chris Peterson suggested we could try calling CFInitialize from the rustc main thread as startup. It's possible that would avoid the crash at library load time but be less invasive than dropping the stack-limiting thread dispatch.

	Ralph Giles (:rillian) Assignee
	Comment 31 • 7 years ago

I've also seen stylo's build script hang on both the ci 10.7 machines and my local mac with 10.12.

	Nathan Froyd [:froydnj] Reporter
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8871483 [details]
Bug 1365993 - Don't pass mac frameworks to cargo linker.

https://reviewboard.mozilla.org/r/142944/#review146968

::: config/rules.mk:1002
(Diff revision 1)
>  # some crates's build scripts (!), so disable it for now.
>  ifndef MOZ_ASAN
>  ifndef MOZ_TSAN
> +# Cargo needs the same linker flags as the C/C++ compiler,
> +# but not the final libraries. Filter those out because
> +# they cause problems on macOS 10.7.

Maybe add "; see bug 1365993 for details."?

::: config/rules.mk:1004
(Diff revision 1)
>  ifndef MOZ_TSAN
> +# Cargo needs the same linker flags as the C/C++ compiler,
> +# but not the final libraries. Filter those out because
> +# they cause problems on macOS 10.7.
>  target_cargo_env_vars := \
> -	MOZ_CARGO_WRAP_LDFLAGS="$(LDFLAGS)" \
> +	MOZ_CARGO_WRAP_LDFLAGS="$(filter-out -framework Cocoa -lobjc AudioToolbox ExceptionHandling,$(LDFLAGS))" \

Just to make sure I understand, we're actually passing `-framework Cocoa -framework AudioToolbox -framework ExceptionHandling -lobjc` (or some ordering thereof) in `LDFLAGS`?

	Ralph Giles (:rillian) Assignee
	Comment 33 • 7 years ago

(In reply to Nathan Froyd [:froydnj] from comment #32)

> Maybe add "; see bug 1365993 for details."?

Good idea.

> Just to make sure I understand, we're actually passing `-framework Cocoa
> -framework AudioToolbox -framework ExceptionHandling -lobjc` (or some
> ordering thereof) in `LDFLAGS`?

Yes. On my local mac build, config.status has

>     'OS_LDFLAGS': '  -framework Cocoa -lobjc -framework AudioToolbox',

from https://dxr.mozilla.org/mozilla-central/source/old-configure.in#2372
and https://dxr.mozilla.org/mozilla-central/source/old-configure.in#2786

The config/config.mk:273 passes OS_LDFLAGS into LDFLAGS before they're used in rules.mk.
https://dxr.mozilla.org/mozilla-central/source/config/config.mk#273

Looking at that now, we could probably remove the uikit branch and move the libraries into moz.build files. (Or TKLIBS? I don't know where those go.) It's probably like this because threading conditional per-target flags through from autoconf is tedious.

	Pulsebot
	Comment 35 • 7 years ago

Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94245f1ea0b5
Don't pass mac frameworks to cargo linker. r=froydnj

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 36 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/94245f1ea0b5