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

windows-gnu: prefer system crt libraries if they are available #67429

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Dec 19, 2019

The origin of the issue is the fact Rust ships mingw-w64 libraries but no headers and prefers own libraries over the system ones.
This leads to situation when headers aren't compatible with libraries (mingw-w64 doesn't provide any forward compatibility and AFAIK backwards compatibility is guaranteed only within major release series).

It's easier to understand how this PR works when looking at the linker invocation before and with this PR: https://www.diffchecker.com/GEuYFmzo
It adds system libraries path before Rust libraries so the linker will prefer them.
It has potential issue when system has files with the same names as Rust but that could be avoided by moving Rust shipped mingw-w64 libraries from lib/rustlib/x86_64-pc-windows-gnu/lib to say lib/rustlib/x86_64-pc-windows-gnu/lib/mingw. Then adding linker paths in this order: Rust libraries, system libraries, Rust shipped mingw-w64 libraries.

Fixes #47048
Fixes #49078
Fixes #53454
Fixes #60912

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2019
@mati865
Copy link
Contributor Author

mati865 commented Dec 19, 2019

CC developers who seem interested in this: @Amanieu @Mark-Simulacrum @nikomatsakis @pnkfelix

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-19T16:35:49.0530929Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-19T16:35:49.0543708Z ##[command]git config gc.auto 0
2019-12-19T16:35:49.0548284Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-19T16:35:49.0552631Z ##[command]git config --get-all http.proxy
2019-12-19T16:35:49.0559037Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67429/merge:refs/remotes/pull/67429/merge
---
2019-12-19T16:39:29.7469980Z   local time: Thu Dec 19 16:39:29 UTC 2019
2019-12-19T16:39:30.2667204Z   network time: Thu, 19 Dec 2019 16:39:30 GMT
2019-12-19T16:39:30.2672651Z == end clock drift check ==
2019-12-19T16:39:43.5235272Z 
2019-12-19T16:39:43.5320460Z ##[error]Bash exited with code '1'.
2019-12-19T16:39:43.5347842Z ##[section]Starting: Checkout
2019-12-19T16:39:43.5350637Z ==============================================================================
2019-12-19T16:39:43.5350708Z Task         : Get sources
2019-12-19T16:39:43.5350746Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@jonas-schievink jonas-schievink added A-linkage Area: linking into static, shared libraries and binaries O-windows-gnu Toolchain: GNU, Operating system: Windows labels Dec 22, 2019
@bors
Copy link
Contributor

bors commented Dec 23, 2019

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

@mati865
Copy link
Contributor Author

mati865 commented Dec 23, 2019

I think this won't work for cross compiling but let's see.

Can somebody give it @bors try?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 27, 2019

@bors try

@bors
Copy link
Contributor

bors commented Dec 27, 2019

⌛ Trying commit 70509f9c64ac81809be8d391edff62ae332b59d7 with merge 252370535bbc6de48a3a93ba93bdf654d02914a6...

@bors
Copy link
Contributor

bors commented Dec 27, 2019

☀️ Try build successful - checks-azure
Build commit: 252370535bbc6de48a3a93ba93bdf654d02914a6 (252370535bbc6de48a3a93ba93bdf654d02914a6)

@mati865
Copy link
Contributor Author

mati865 commented Dec 27, 2019

Tested and confirmed that this PR works well on Windows but when cross compiling it still picks bundled crt2.o.

I don't have idea how to modify all those #[cfg(all(windows, target_env = "gnu"))] so they do work when cross compiling from and could use some help.

@mati865 mati865 marked this pull request as ready for review December 27, 2019 17:34
@jonas-schievink
Copy link
Contributor

The Session object should contain info about the target platform

@mati865
Copy link
Contributor Author

mati865 commented Dec 27, 2019

@jonas-schievink
Copy link
Contributor

I don't think you need a lazy_static there, a regular (function-scoped) static should suffice. Then the function can fill in the data when called for the first time.

@mati865
Copy link
Contributor Author

mati865 commented Dec 27, 2019

@jonas-schievink wouldn't that involve static mut which is "bad"(#53639)?

@jonas-schievink
Copy link
Contributor

Ah, right. If you can use the parking_lot Mutex you can const-construct it and don't need a static mut.

I think you can do the same with the current code though, by removing the #[cfg]s from most stuff. Then you can make use of the Session to detect the target platform.

@mati865
Copy link
Contributor Author

mati865 commented Dec 29, 2019

Okay, this works fine on native Windows.

I'd like to test cross compilation so can somebody do @bors try?

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Dec 29, 2019

⌛ Trying commit 0e093bb with merge 4069647...

bors added a commit that referenced this pull request Dec 29, 2019
[WIP] windows-gnu: prefer system crt libraries if they are available

This is my proposal (based on `Amanieu`'s idea) on how to fix #47048 and related issues.

The origin of the issue is the fact Rust ships mingw-w64 libraries but no headers and prefers own libraries over the system ones.
This leads to situation when headers aren't compatible with libraries (mingw-w64 doesn't provide any forward compatibility and AFAIK backwards compatibility is guaranteed only within major release series).

It's easier to understand how this PR works when looking at the linker invocation before and with this PR: https://www.diffchecker.com/GEuYFmzo
It adds system libraries path before Rust libraries so the linker will prefer them.
It has potential issue when system has files with the same names as Rust but that could be avoided by moving Rust shipped mingw-w64 libraries from `lib/rustlib/x86_64-pc-windows-gnu/lib` to say `lib/rustlib/x86_64-pc-windows-gnu/lib/mingw`. Then adding linker paths in this order: Rust libraries, system libraries, Rust shipped mingw-w64 libraries.

I don't know if it's worth to cache system libraries path. You can look for `cache: ` string during build Rust: https://pastebin.com/kGEQZGWP
I think there are enough calls to justify caching.

Obvious shortcoming here is this version works only on windows-gnu host because of `#[cfg(all(windows, target_env = "gnu"))]` but you should get the idea.
@bors
Copy link
Contributor

bors commented Dec 29, 2019

☀️ Try build successful - checks-azure
Build commit: 4069647 (406964755288e3d712edc2347b8b2f298900fe9a)

bors added a commit that referenced this pull request Feb 5, 2020
windows-gnu: prefer system crt libraries if they are available

The origin of the issue is the fact Rust ships mingw-w64 libraries but no headers and prefers own libraries over the system ones.
This leads to situation when headers aren't compatible with libraries (mingw-w64 doesn't provide any forward compatibility and AFAIK backwards compatibility is guaranteed only within major release series).

It's easier to understand how this PR works when looking at the linker invocation before and with this PR: https://www.diffchecker.com/GEuYFmzo
It adds system libraries path before Rust libraries so the linker will prefer them.
It has potential issue when system has files with the same names as Rust but that could be avoided by moving Rust shipped mingw-w64 libraries from `lib/rustlib/x86_64-pc-windows-gnu/lib` to say `lib/rustlib/x86_64-pc-windows-gnu/lib/mingw`. Then adding linker paths in this order: Rust libraries, system libraries, Rust shipped mingw-w64 libraries.

Fixes #47048
Fixes #49078
Fixes #53454
Fixes #60912
@bors
Copy link
Contributor

bors commented Feb 5, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 58b8343 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 5, 2020
@bors bors merged commit 1fad337 into rust-lang:master Feb 5, 2020
@mati865 mati865 deleted the mingw-ultimate-fix branch February 5, 2020 22:43
@tanriol
Copy link
Contributor

tanriol commented Feb 6, 2020

What's the proper way to make this work for systems that do not match this pattern? For example, on my Gentoo Linux system the cross-compiler /usr/bin/x86_64-w64-mingw32-gcc is a symlink to /usr/x86_64-pc-linux-gnu/x86_64-w64-mingw32/gcc-bin/9.2.0/x86_64-w64-mingw32-gcc, while the mingw libs live at /usr/x86_64-w64-mingw32/usr/lib/.

@mati865
Copy link
Contributor Author

mati865 commented Feb 6, 2020

@tanriol that's good question.
I don't think it's neither possible or good idea to cover all possible variants.
Maybe it'd be possible to reuse one of Cargo configuration fields or add new one to specify path of system libraries.
Could you open new issue to track this?

lu-zero added a commit to rust-av/rav1e that referenced this pull request Mar 24, 2020
lu-zero added a commit to rust-av/rav1e that referenced this pull request Mar 24, 2020
lu-zero added a commit to xiph/rav1e that referenced this pull request Mar 24, 2020
lu-zero added a commit to xiph/rav1e that referenced this pull request Mar 24, 2020
mati865 referenced this pull request in msys2/MINGW-packages Apr 26, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 16, 2020
Pkgsrc changes:
 * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the
   bootstrap is not (yet?) available.

Upstream changes:

Version 1.43.0 (2020-04-23)
==========================

Language
--------
- [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having
  the type inferred correctly.][68129]
- [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201]

**Syntax only changes**
- [Allow `type Foo: Ord` syntactically.][69361]
- [Fuse associated and extern items up to defaultness.][69194]
- [Syntactically allow `self` in all `fn` contexts.][68764]
- [Merge `fn` syntax + cleanup item parsing.][68728]
- [`item` macro fragments can be interpolated into `trait`s, `impl`s,
  and `extern` blocks.][69366]
  For example, you may now write:
  ```rust
  macro_rules! mac_trait {
      ($i:item) => {
          trait T { $i }
      }
  }
  mac_trait! {
      fn foo() {}
  }
  ```
These are still rejected *semantically*, so you will likely receive an error but
these changes can be seen and parsed by macros and
conditional compilation.


Compiler
--------
- [You can now pass multiple lint flags to rustc to override the previous
  flags.][67885] For example; `rustc -D unused -A unused-variables` denies
  everything in the `unused` lint group except `unused-variables` which
  is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies
  everything in the `unused` lint group **including** `unused-variables` since
  the allow flag is specified before the deny flag (and therefore overridden).
- [rustc will now prefer your system MinGW libraries over its bundled libraries
  if they are available on `windows-gnu`.][67429]
- [rustc now buffers errors/warnings printed in JSON.][69227]

Libraries
---------
- [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement
  `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>`
  respectively.][69538] **Note** These conversions are only available when `N`
  is `0..=32`.
- [You can now use associated constants on floats and integers directly, rather
  than having to import the module.][68952] e.g. You can now write `u32::MAX` or
  `f32::NAN` with no imports.
- [`u8::is_ascii` is now `const`.][68984]
- [`String` now implements `AsMut<str>`.][68742]
- [Added the `primitive` module to `std` and `core`.][67637] This module
  reexports Rust's primitive types. This is mainly useful in macros
  where you want avoid these types being shadowed.
- [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642]
- [`string::FromUtf8Error` now implements `Clone + Eq`.][68738]

Stabilized APIs
---------------
- [`Once::is_completed`]
- [`f32::LOG10_2`]
- [`f32::LOG2_10`]
- [`f64::LOG10_2`]
- [`f64::LOG2_10`]
- [`iter::once_with`]

Cargo
-----
- [You can now set config `[profile]`s in your `.cargo/config`, or through
  your environment.][cargo/7823]
- [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's
  executable path when running integration tests or benchmarks.][cargo/7697]
  `<name>` is the name of your binary as-is e.g. If you wanted the executable
  path for a binary named `my-program`you would use
  `env!("CARGO_BIN_EXE_my-program")`.

Misc
----
- [Certain checks in the `const_err` lint were deemed unrelated to const
  evaluation][69185], and have been moved to the `unconditional_panic` and
  `arithmetic_overflow` lints.

Compatibility Notes
-------------------

- [Having trailing syntax in the `assert!` macro is now a hard error.][69548]
  This has been a warning since 1.36.0.
- [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly
  led to some instances being accepted, and now correctly emits a hard error.

[69340]: rust-lang/rust#69340

Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of `rustc` and
related tools.

- [All components are now built with `opt-level=3` instead of `2`.][67878]
- [Improved how rustc generates drop code.][67332]
- [Improved performance from `#[inline]`-ing certain hot functions.][69256]
- [traits: preallocate 2 Vecs of known initial size][69022]
- [Avoid exponential behaviour when relating types][68772]
- [Skip `Drop` terminators for enum variants without drop glue][68943]
- [Improve performance of coherence checks][68966]
- [Deduplicate types in the generator witness][68672]
- [Invert control in struct_lint_level.][68725]

[67332]: rust-lang/rust#67332
[67429]: rust-lang/rust#67429
[67637]: rust-lang/rust#67637
[67642]: rust-lang/rust#67642
[67878]: rust-lang/rust#67878
[67885]: rust-lang/rust#67885
[68129]: rust-lang/rust#68129
[68672]: rust-lang/rust#68672
[68725]: rust-lang/rust#68725
[68728]: rust-lang/rust#68728
[68738]: rust-lang/rust#68738
[68742]: rust-lang/rust#68742
[68764]: rust-lang/rust#68764
[68772]: rust-lang/rust#68772
[68943]: rust-lang/rust#68943
[68952]: rust-lang/rust#68952
[68966]: rust-lang/rust#68966
[68984]: rust-lang/rust#68984
[69022]: rust-lang/rust#69022
[69185]: rust-lang/rust#69185
[69194]: rust-lang/rust#69194
[69201]: rust-lang/rust#69201
[69227]: rust-lang/rust#69227
[69548]: rust-lang/rust#69548
[69256]: rust-lang/rust#69256
[69361]: rust-lang/rust#69361
[69366]: rust-lang/rust#69366
[69538]: rust-lang/rust#69538
[cargo/7823]: rust-lang/cargo#7823
[cargo/7697]: rust-lang/cargo#7697
[`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed
[`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html
[`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html
[`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html
[`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html
[`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
barrbrain pushed a commit to barrbrain/rav1e that referenced this pull request May 26, 2020
barrbrain pushed a commit to xiph/rav1e that referenced this pull request May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries merged-by-bors This PR was explicitly merged by bors. O-windows-gnu Toolchain: GNU, Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet