Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invoke backtrace-rs buildscript in std buildscript #116318

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Oct 1, 2023

@rustbot
Copy link
Collaborator

rustbot commented Oct 1, 2023

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 1, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@pitaj
Copy link
Contributor Author

pitaj commented Oct 1, 2023

I believe jubilee is in charge of backtrace-rs now

r? workingjubilee

@workingjubilee
Copy link
Member

effectively.
rebase plz.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-15 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@workingjubilee
Copy link
Member

Alright, this is waiting on rust-lang/cc-rs#705 as I understand it?

@workingjubilee workingjubilee added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2023
@ChrisDenton
Copy link
Member

I can take another look at that PR. IIRC I was happy to merge it but it'd be nice if someone with Android experience could give it a quick test since the PR author has been busy. Unfortunately I don't have a lot of faith in cc's testing atm.

@workingjubilee
Copy link
Member

Ping one of the Android target maintainers? They're usually pretty on-the-ball.

@pitaj
Copy link
Contributor Author

pitaj commented Oct 3, 2023

This isn't technically blocked by the cc change, but that PR is the last piece of the puzzle to fix the Android issue.

Whether to merge this now or later I'll leave up to you.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 3, 2023

given:

I kinda forgot about this PR again (whoops) I'll actually take a look sometime tomorrow morning

I think we can at least wait for some cleanup/doublechecking to happen assuming that it doesn't grow excessively in time.

@workingjubilee workingjubilee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 5, 2023
@workingjubilee
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 6, 2023

📌 Commit 9367727 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2023
@bors
Copy link
Contributor

bors commented Oct 7, 2023

⌛ Testing commit 9367727 with merge 4ea5190...

@ChrisDenton
Copy link
Member

ChrisDenton commented Oct 13, 2023

An option to say "use this API level" would be ideal. A cfg flag to do this is what I am currently prototyping. I have a working build with the following changes:

A cfg flag to set the API level sounds related to rust-lang/rfcs#3379. E.g. rustc --cfg 'target_os_version.android="21"' Which is the latest iteration of something that keeps getting proposed but seems to keep getting stalled.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

At first I tried encoding the version as a value in cfg but then it's really hard to use, cuz the #[cfg] matchers only let you check for equality, which is why I ended up with the much less pretty android_api_at_least_21 flag.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

I can throw up a PR with what I have working locally to discuss there if you'd like?

Yes, I think that'd be good to see.

Here is what I was using to make this work locally: rust-lang/backtrace-rs#570

To the question above though: If we could set the Min API level to 21, then all of this can just go away.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

https://blog.rust-lang.org/2023/01/09/android-ndk-update-r25.html, published January 2023 says:

We are pleased to announce that Android platform support in Rust will be modernized in Rust 1.68 as we update the target NDK from r17 to r25. As a consequence the minimum supported API level will increase from 15 (Ice Cream Sandwich) to 19 (KitKat).

25 is still in use on CI:

download_ndk android-ndk-r25b-linux.zip

@workingjubilee
Copy link
Member

Yeah, okay. And the latest NDK still supports API L19 and L20, but the next one will be L21 as its minimum, so that only hits the table after it releases.

@workingjubilee
Copy link
Member

...wait, did r26 get released? when?

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

@pitaj
Copy link
Contributor Author

pitaj commented Oct 13, 2023

I'm confused as to why just including extern crate cc; would cause problems. Can you share the exact error you're seeing?

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

Absolutely, here's the error when I remove the --cfg from my prototype PR:

error[E0463]: can't find crate for `cc`
  --> ../../third_party/rust-toolchain/lib/rustlib/src/rust/library/std/../backtrace/build.rs:14:5
   |
14 |     extern crate cc;
   |     ^^^^^^^^^^^^^^^^ can't find crate

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
  --> ../../third_party/rust-toolchain/lib/rustlib/src/rust/library/std/../backtrace/build.rs:29:9
   |
29 |     let expansion = match cc::Build::new().file(&android_api_c).try_expand() {
   |         ^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `[u8]`
   = note: all local variables must have a statically known size
   = help: unsized locals are gated as an unstable feature

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
  --> ../../third_party/rust-toolchain/lib/rustlib/src/rust/library/std/../backtrace/build.rs:30:12
   |
30 |         Ok(result) => result,
   |            ^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `[u8]`
   = note: all local variables must have a statically known size
   = help: unsized locals are gated as an unstable feature

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
  --> ../../third_party/rust-toolchain/lib/rustlib/src/rust/library/std/../backtrace/build.rs:30:9
   |
30 |         Ok(result) => result,
   |         ^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `[u8]`
note: required by a bound in `Ok`
  --> /b/s/w/ir/cache/builder/src/third_party/rust-src/library/core/src/result.rs:506:5

error: aborting due to 4 previous errors

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

I suspect what's confusing here is that the cc crate is simply not present in our build tree when building the stdlib as part of the Chromium build. So there's no path to it given to rustc.

@workingjubilee
Copy link
Member

cc @chriswailes since we're currently discussing possible Android build hacks for std and min API levels.

@pitaj
Copy link
Contributor Author

pitaj commented Oct 13, 2023

@danakj have you tried making cc available in your build? Seems like build dependencies should at least be available even if they aren't used.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

Yeah, we can not use cc as we can not have build-graph edges introduced outside of our build system. Compiler invocations have to come from our build system. When running the build script, we also may not even have a compiler present. We do all build steps separately from execution.

Edit: Build dependencies in general are available, but cc is excluded because it would be very incorrect to use it. So we prefer to get compilation errors rather than runtime errors.

Edit edit: In this case, a runtime error may not have been noticed, but we would have received API level < 21 as a result, which would be incorrect, for example.

@pitaj
Copy link
Contributor Author

pitaj commented Oct 13, 2023

I know you can't use cc, and I'm not proposing that you do. But making it available, even if it isn't invoked, will sidestep the "need to cfg out extern crate cc;" issue.

Right, we simply provide possibly incorrect results if invoking cc fails.

But if you define the environment variable as proposed earlier, I think everything would work fine (and cc would not be invoked at all).

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

Right, but I am not sure how to ensure that we don't accidentally invoke cc (which would use the wrong compiler entirely if any), or end up with incorrect decisions, if the crate is present. Compiler errors ensure we remain aware of any attempts to use cc. Am I overlooking something that would help with that?

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2023
Revert "Invoke `backtrace-rs` buildscript in `std` buildscript"

This reverts commit 9367727 because it caused issues for projects building the standard library with non-cargo build systems.

See rust-lang#116318 (comment)

r? workingjubilee
@pitaj
Copy link
Contributor Author

pitaj commented Oct 13, 2023

I thought you said you don't even need backtrace-rs anyways, so is having the wrong android version even an issue? I guess you don't want cc to be available for anything else either.

Regardless, I think my preferred option, if you need it, would be a --cfg no_cc which requires the ANDROID_VERSION environment variable.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

Yeah, I am concerned about making our build system fragile by adding cc to it, in which case we won't hear about future uses of it so that we can provide a static replacement. I agree making it fail to run in this particular instance would be fine.

A no_cc cfg flag sounds totally great to me.

@RalfJung
Copy link
Member

RalfJung commented Oct 13, 2023

You could replace cc by a crate that panics in cc::Build::new. The build script can then check at runtime whether it wants to invoke cc or not, but there's no need to make the build script build fail if it doesn't actually call cc.

Everything you'd do with compile-time checks like cfg in regular crates will be run-time checks in build.rs, since it runs at build time. So cfg would be replaced by regular if. In such a world, refusing to even build cc is going way too far IMO. I don't think it is reasonable to expect std's build script to never link cc. Never executing it is a different matter, that can be arranged much easier.

You will still get your errors at build time since a panicking build script will stop the build.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

I did consider replicating the entire cc API in a stub crate before, and even prototyped that. In order to compile you need all of the public API available though, and then match changes across versions. It's not as simple as just making a cc::Build::new unfortunately. Removing use of cc has thus far been much more straightforward.

@pitaj
Copy link
Contributor Author

pitaj commented Oct 13, 2023

Instead of replicating the entire API, it would probably be easier to just fork the existing source code and just adding panics in a couple places.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

Yeah, this is possible, though it requires maintaining a fork, possibly multiple versions over time, and shimming a different library into the build system. The complexity is larger than removing it entirely. This will need to be replicated for each project building stdlib in a static build system.

I understand the no_cc flag represents some complexity on your end that it avoids. Is it too much?

@workingjubilee
Copy link
Member

I don't think a no_cc flag is necessarily too much, but I still want to reevaluate the premise though, because if our Android targets should be moving to API Level 21 soon anyways, then that's another approach.

@danakj
Copy link
Contributor

danakj commented Oct 13, 2023

FWIW Here's what it could look like to use __ANDROID_API__ from the environment when no_cc is set: rust-lang/backtrace-rs#570

@jyn514
Copy link
Member

jyn514 commented Oct 14, 2023

Wow, in build scripts, we just can't cfg based on the actual target at all, huh. That's... really, really annoying.

Yes you can. https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts

nevermind that does runtime switching, i didn't realize the build script was using extern crate cc

@danakj
Copy link
Contributor

danakj commented Oct 19, 2023

What should be the next step here? We're currently on a rustc nearing a month old, which prevents us from finding other issues, like LLVM head integration fixes for example, as they stack up.

Can we roll the NDK and then the minimum ABI will be 21 and this goes away? If so, who would do that, is there an approval process for it?

If not, could we hold the no_cc option until the NDK can roll, so that we (and others not using cargo) can continue to build the stdlib without the cc dependency? (We remove the dep from the toml downstream, programatically not by patching, it's only the rs usage that is a problem.)

@bjorn3
Copy link
Member

bjorn3 commented Oct 19, 2023

Would temporarily apply the patch in #116705 (which reverts this PR) when using rustc 1.73 work? It landed a week ago and would end up on stable December 28. (or November 16 if backported to beta)

Edit: I just see that this PR hasn't even landed on beta yet, so no beta backport of the revert is necessary.

@danakj
Copy link
Contributor

danakj commented Oct 19, 2023

Oh I didn't see the revert, I see it in the comment thread above now. Then we will roll past that, yeah. Thank you!

The next step question still applies, and I am happy to participate in what comes next, but that will unblock us, thanks!

@danakj
Copy link
Contributor

danakj commented Nov 10, 2023

FWIW, Chromium's minimum API level is now 26: https://groups.google.com/a/chromium.org/g/chromium-dev/c/B9AYI3WAvRo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.