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

doc: discuss build script instruction order #10600

Merged
merged 2 commits into from
May 25, 2022

Conversation

tmfink
Copy link
Contributor

@tmfink tmfink commented Apr 26, 2022

What does this PR try to resolve?

It is currently not documented that the order of build script cargo: instructions may be relevant to linking.

This has caused issues such as: rust-lang/rust#96328

How should we test and review this PR?

Build/view documentation.

Additional information

  • Cargo maintainers should fact check my wording.
  • We may need to discuss if this should also be documented for rustc
  • Maintainers should ensure that this change does not prevent a change in what is currently unspecified behavior. Perhaps cargo will want to rearrange link arguments itself to resolve issues in the future?

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2022
@ehuss
Copy link
Contributor

ehuss commented Apr 29, 2022

I'm a little uneasy about making statements about the ordering, though in rust-lang/rust#96328 (comment) it seems to imply this is ok. @petrochenkov, do you think the statements made in this PR are reasonable?

I would also feel a little more comfortable if there is a test to ensure the order is preserved (at least as passed into rustc). Can you maybe include something like the following?

diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs
index d0ef3a2e7..8ca259d1e 100644
--- a/tests/testsuite/build_script.rs
+++ b/tests/testsuite/build_script.rs
@@ -1789,10 +1789,13 @@ fn output_separate_lines_new() {
                 fn main() {
                     println!("cargo:rustc-link-search=foo");
                     println!("cargo:rustc-link-lib=static=foo");
+                    println!("cargo:rustc-link-lib=bar");
+                    println!("cargo:rustc-link-search=bar");
                 }
             "#,
         )
         .build();
+    // The order of the arguments passed to rustc is important.
     p.cargo("build -v")
         .with_status(101)
         .with_stderr_contains(
@@ -1800,7 +1803,7 @@ fn output_separate_lines_new() {
 [COMPILING] foo v0.5.0 ([CWD])
 [RUNNING] `rustc [..] build.rs [..]`
 [RUNNING] `[..]/foo-[..]/build-script-build`
-[RUNNING] `rustc --crate-name foo [..] -L foo -l static=foo`
+[RUNNING] `rustc --crate-name foo [..] -L foo -L bar -l static=foo -l bar`
 [ERROR] could not find native static library [..]
 ",
         )

@tmfink
Copy link
Contributor Author

tmfink commented Apr 30, 2022

I'm a little uneasy about making statements about the ordering, though in rust-lang/rust#96328 (comment) it seems to imply this is ok. @petrochenkov, do you think the statements made in this PR are reasonable?

I tried to avoid being too strong with my wording by using IETF RFC2119-style language like:

... may affect the order of arguments.

I understand that certain behavior may change, such rustc and/or cargo trying to optimize order of arguments. However, this is a problem today (as shown by rust-lang/rust#96328). To make the added text more clear, we could add language along the lines of "this information is subject to change in future releases of rustc/cargo.

I could also clarify the example by saying, "For example, Unix-like systems often require dynamic libraries to listed after objects that depend on them." Different platforms (real, imaginary, or yet-to-be invented) may not care about order or even require the opposite order.

@tmfink
Copy link
Contributor Author

tmfink commented Apr 30, 2022

Side note: both cargo and rustc potentially affect the final order of arguments passed to the linker.
Should this also be documented somewhere for rustc?

@tmfink tmfink force-pushed the doc-build-script-link-order branch from 1fdb47b to ac2cba8 Compare May 2, 2022 00:21
@petrochenkov
Copy link
Contributor

@ehuss

do you think the statements made in this PR are reasonable?

Yes, I think they are reasonable.
Native library ordering is important during linking in practice, and therefore it's important for cargo to guarantee that it's preserved.

@tmfink tmfink force-pushed the doc-build-script-link-order branch from ac2cba8 to 43ce1e7 Compare May 24, 2022 19:46
@ehuss
Copy link
Contributor

ehuss commented May 25, 2022

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2022

📌 Commit 43ce1e7 has been approved by ehuss

@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 May 25, 2022
@bors
Copy link
Contributor

bors commented May 25, 2022

⌛ Testing commit 43ce1e7 with merge 39ad103...

@bors
Copy link
Contributor

bors commented May 25, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 39ad103 to master...

@bors bors merged commit 39ad103 into rust-lang:master May 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2022
Update cargo

10 commits in a4c1cd0eb6b18082a7e693f5a665548fe1534be4..39ad1039d9e3e1746177bf5d134af4c164f95528
2022-05-20 00:55:25 +0000 to 2022-05-25 00:50:02 +0000

* doc: discuss build script instruction order (rust-lang/cargo#10600)
* Require http-registry URLs to end with a '/' (rust-lang/cargo#10698)
* No printing executable names when running tests and benchmarks with json message format (rust-lang/cargo#10691)
* Restore proper error for crate not in local reg (rust-lang/cargo#10683)
* Update libcurl (rust-lang/cargo#10696)
* Fixed small typos (rust-lang/cargo#10693)
* fix bugs with `workspace` key and `update_toml` (rust-lang/cargo#10685)
* Bump to 0.64.0, update changelog (rust-lang/cargo#10687)
* List C compiler as a build dependency in README (rust-lang/cargo#10678)
* Add unstable `rustc-check-cfg` build script output (rust-lang/cargo#10539)

r? `@ehuss`
@ehuss ehuss added this to the 1.63.0 milestone Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants