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

Add windows-gnu CI and fix tests #8139

Merged
merged 4 commits into from
Apr 21, 2020
Merged

Add windows-gnu CI and fix tests #8139

merged 4 commits into from
Apr 21, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 20, 2020

One remaining failure:

---- features::feature_off_dylib stdout ----
running `d:\a\1\s\target\debug\cargo.exe build --features f1`
running `d:\a\1\s\target\debug\cargo.exe run -p bar`
thread 'features::feature_off_dylib' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout

--- stderr
   Compiling foo v0.0.1 (D:\a\1\s\target\cit\t663\foo)
   Compiling bar v0.0.1 (D:\a\1\s\target\cit\t663\foo\bar)
    Finished dev [unoptimized + debuginfo] target(s) in 0.69s
     Running `target\debug\bar.exe`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"f1"`,
 right: `"no f1"`', bar\src\main.rs:5:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\bar.exe` (exit code: 101)
', crates\cargo-test-support\src\lib.rs:833:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    features::feature_off_dylib

I disassembled the dylibs and cargo run -p bar correctly rebuilt it inside target/debug/deps/ but did not copy it to target/debug. To further confirm, calling cp target/debug/deps/foo.dll target/debug/ manually solved the issue.
Any idea?


I left FIXME in places where import lib should be added with #6875.

TOOLCHAIN: nightly-x86_64-pc-windows-gnu can be replaced with beta on Thursday.

@rust-highfive
Copy link

r? @Eh2406

(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 Apr 20, 2020
@mati865
Copy link
Contributor Author

mati865 commented Apr 20, 2020

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Apr 20, 2020
@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2020

Hm, so there's more detail in #6167 where that test was introduced. Dependency DLL's are not copied into target/debug. This can be a problem if there are DLL's in that directory from running other cargo commands (see #6162 for an example where that can happen). The fix that was implemented relies on the order of directories in PATH (target/debug/deps is first, which contains all the latest DLLs).

I'm a bit confused why the gnu toolchain has a different search order. I tried to find something useful in https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order, but that only confused me more. It says "The directory from which the application loaded." is first, so I would assume that test should fail with MSVC (we launch the executable in target/debug which has the old foo.dll).

So, anyways, I'm guessing something in the GNU setup is changing the search order to include the binary's directory before PATH.

One possibility is to go with option 3 described at #6162 (comment), where all dependency DLLs should be copied to target/debug. I'm not sure how hard that is to implement.

"Uplifting" is done here. The outputs of each unit are computed in calc_outputs_rustc. The decision to only uplift the "root" units is done here. Maybe that should also say || unit.target.is_dylib()? That way every DLL for each dependency will get copied to target/debug.

# FIXME: switch to stable when 1.44 is released
TOOLCHAIN: nightly-x86_64-pc-windows-gnu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep this on nightly permanently, so that we have some nightly testing for Windows (which enables a bunch of tests that are usually ignored).

@mati865
Copy link
Contributor Author

mati865 commented Apr 21, 2020

MinGW and MSVC both use the same library loader from the OS.

Somehow MSVC creates hardlinks but MinGW copies foo.dll:

$ fsutil hardlink list target/debug/foo.dll
\cargo\target\cit\t663\foo\target\debug\foo.dll

$ fsutil hardlink list target/x86_64-pc-windows-msvc/debug/foo.dll
\cargo\target\cit\t663\foo\target\x86_64-pc-windows-msvc\debug\deps\foo.dll
\cargo\target\cit\t663\foo\target\x86_64-pc-windows-msvc\debug\foo.dll

EDIT:
Hardlink is created properly but cargo run -p bar unlinks it on windows-gnu.

@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2020

Ah, good observation. My guess would be a difference in linker behavior. In the presence of an existing file, MSVC probably truncates and modifies, preserving the hardlink, whereas gnu probably replaces the file, breaking the hardlink (at least, that's my guess).

Awesome, thanks for helping!

@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2020

📌 Commit 2dd0b0d 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 Apr 21, 2020
@bors
Copy link
Contributor

bors commented Apr 21, 2020

⌛ Testing commit 2dd0b0d with merge 97d39ae...

@mati865
Copy link
Contributor Author

mati865 commented Apr 21, 2020

In the presence of an existing file, MSVC probably truncates and modifies, preserving the hardlink, whereas gnu probably replaces the file, breaking the hardlink (at least, that's my guess).

I had tested LLD before and it failed the same way with gnu target. Lld-link with msvc target had no errors.

@bors
Copy link
Contributor

bors commented Apr 21, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 97d39ae to master...

@bors bors merged commit 97d39ae into rust-lang:master Apr 21, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2020
Update cargo, rls

## cargo

17 commits in ebda5065ee8a1e46801380abcbac21a25bc7e755..8751eb3010d4cdb5329b5a6bd2b6d765c95b0dca
2020-04-16 14:28:43 +0000 to 2020-04-21 18:04:35 +0000
- Uplift windows gnu DLL import libraries. (rust-lang/cargo#8141)
- Add windows-gnu CI and fix tests (rust-lang/cargo#8139)
- Several updates to token/index handling. (rust-lang/cargo#7973)
- Add `resolver` opt-in for new feature resolver. (rust-lang/cargo#8129)
- Improve error message when running `cargo install .` (rust-lang/cargo#8137)
- fix mem replace unused (rust-lang/cargo#8138)
- Change `-Cembed-bitcode=no` use to `-Cbitcode-in-rlib=no`. (rust-lang/cargo#8134)
- Refactor BuildContext (rust-lang/cargo#8068)
- Rename allows_underscores to allows_dashes. (rust-lang/cargo#8135)
- Fixed a needless borrow. (rust-lang/cargo#8130)
- Add link to changelog in the Cargo book. (rust-lang/cargo#8126)
- Fix target for doc test cross compilation (rust-lang/cargo#8094)
- Add note about .cargo/config support. (rust-lang/cargo#8125)
- Fix pdb uplift when executable has dashes. (rust-lang/cargo#8123)
- Hint upgrading for future edition keys (rust-lang/cargo#8122)
- Use some fs shorthand functions. (rust-lang/cargo#8124)
- Update documentation to mention "config.toml" instead of "config" (rust-lang/cargo#8121)

## rls

1 commits in 2659cbf14bfb0929a16d7ce9b6858d0bb286ede7..7de2a1f299f8744ffe109139f9f1fdf28bfec909
2020-04-14 22:07:24 +0200 to 2020-04-19 22:41:55 +0000
- Update cargo (rust-lang/rls#1663)
@mati865 mati865 deleted the patch-1 branch January 26, 2022 23:56
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 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