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

build-override results in the same dependency being compiled twice #12645

Closed
link2xt opened this issue Sep 8, 2023 · 6 comments
Closed

build-override results in the same dependency being compiled twice #12645

link2xt opened this issue Sep 8, 2023 · 6 comments
Labels
A-build-scripts Area: build.rs scripts A-profiles Area: profiles C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@link2xt
Copy link

link2xt commented Sep 8, 2023

Problem

I am compiling tag v1.121.0 of https://github.com/deltachat/deltachat-core-rust with command cargo build. It compiles aho-corasick v1.0.2 dependency twice, once as a dependency and once as a build dependency, resulting in two .rlib field being produced and actually slowing down the build process rather than speeding it up as intended by the build-override feature.

Steps

  1. Clone https://github.com/deltachat/deltachat-core-rust, tag v1.121.0, commit a8551510cdb5be7a35ebd3574df79d15a4fa471a
  2. Run cargo build -v.
  3. Observe aho-corasick v1.0.2 dependency being compiled twice. Once as a proper dependency with -C panic=abort as specified in the manifest, and once as a build dependency without -C panic=abort.
  4. Try to specify panic='abort' in the [profile.dev.build-override] section of Cargo.toml as a workaround, it results in an error "panic may not be specified in a build-override profile".

Possible Solution(s)

Do not apply build-override to dependencies that are actual dependencies of the project? Or allow to disable build-override completely.

Notes

I wrote a longer thread with my observations of the build times and duplicate builds of dependencies at https://users.rust-lang.org/t/same-dependency-has-four-different-debug-fingerprints/99580

Version

$ cargo version --verbose
cargo 1.72.0
release: 1.72.0
host: x86_64-unknown-linux-gnu
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 8.2.1 (sys:0.4.63+curl-8.1.2 system ssl:OpenSSL/3.1.2)
os: Arch Linux [64-bit]
@link2xt link2xt added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Sep 8, 2023
@weihanglo weihanglo added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Sep 8, 2023
@weihanglo
Copy link
Member

That's a consequence of this setting, not build-override. Guess you just had a typo 🤔.

By default, the panic strategy applies to build scripts and build dependencies is panic = "unwind". This cannot be overridden. Here are some prior discussions.

Compiler settings of build scripts are usually different from normal dependencies. This is kind of a tradeoff and somebody may run into the bad part. However, I don't really know why we cannot force change panic for build scripts. Let's nominate this to discuss.

@weihanglo weihanglo added A-profiles Area: profiles A-build-scripts Area: build.rs scripts S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Sep 8, 2023
@link2xt
Copy link
Author

link2xt commented Sep 8, 2023

Setting panic = 'abort' is on purpose to reduce the binary size and is not a typo. unwind is not really useful for us as deltachat-core-rust runs either on mobile or in a GUI application and user will not see the stacktrace on panic anyway. We have got rid of all panicking code and have not seen panic for a long time, so supporting stacktraces is just a waste of space.

Prior discussions are about tests which probably makes sense because it is common to use assertions that panic in the tests instead of returning Result, but for build scripts I would be totally fine if they just abort on panic.

@weihanglo
Copy link
Member

Sorry not being clear. I meant the title is a bit misleading. It's about the panic setting always sticking to unwind. If that is the case, could you update to something like "support panic for build-override profile"?

@link2xt
Copy link
Author

link2xt commented Sep 8, 2023

Sorry not being clear. I meant the title is a bit misleading. It's about the panic setting always sticking to unwind. If that is the case, could you update to something like "support panic for build-override profile"?

Fourth step to reproduce (trying to set panic to abort) is more like a workaround, I was trying to unify the profiles manually. But ideally this should not be necessary, cargo could detect automatically that dependency is used as a normal dependency and build dependency and use normal dependency built will all optimizations without the build override. So the issue is not just about the ability to build a build dependency with panic='abort', but cargo not applying the override at all when it does not reduce the build time.

@link2xt
Copy link
Author

link2xt commented Sep 8, 2023

I have created a separate proposal #12647 to track panic validation for build-override.

@weihanglo weihanglo removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Sep 11, 2023
@weihanglo
Copy link
Member

cargo could detect automatically that dependency is used as a normal dependency and build dependency and use normal dependency built will all optimizations without the build override

As ehuss said in #12647 (comment), it's unlikely for Cargo to know whether a build script could be built with panic=abort, given a build script can have arbitrary code. At this time, lto, rpath, panic are kinda global settings that you cannot set a different value for a single dependency. For other profile settings, build artifacts should be shared between normal and build in usual situations.

Going to close this as not planned, but let us know if you've got new thought to resolve the limitation.

@weihanglo weihanglo closed this as not planned Won't fix, can't repro, duplicate, stale Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-profiles Area: profiles C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

2 participants