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

Fix windows link error #208

Closed
wants to merge 33 commits into from
Closed

Fix windows link error #208

wants to merge 33 commits into from

Conversation

ilslv
Copy link
Member

@ilslv ilslv commented Mar 10, 2022

Synopsis

CI job for windows has linking error.

Solution

#208 (comment)

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@ilslv ilslv self-assigned this Mar 10, 2022
@ilslv ilslv added the enhancement Improvement of existing features or bugfix label Mar 10, 2022
@ilslv
Copy link
Member Author

ilslv commented Mar 10, 2022

Finally some movement. Linking error is present after update parking_lot to 0.12 and definitely related to switch from winapi to windows-rs. Cargo.lock diffs:

[[package]]
name = "parking_lot"
- version = "0.11.2"
+ version = "0.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"

[[package]]
name = "parking_lot_core"
- version = "0.8.5"
+ version = "0.9.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
 "cfg-if",
-  "instant",		
 "libc",
 "redox_syscall",
- "smallvec",
- "winapi",
+ "windows-sys",
]
+ 
+ [[package]]
+ name = "windows-sys"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"
+ dependencies = [
+  "windows_aarch64_msvc",
+  "windows_i686_gnu",
+  "windows_i686_msvc",
+  "windows_x86_64_gnu",
+  "windows_x86_64_msvc",
+ ]
+ [[package]]
+ name = "windows_aarch64_msvc"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"
+ [[package]]
+ name = "windows_i686_gnu"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"
+ [[package]]
+ name = "windows_i686_msvc"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"
+ [[package]]
+ name = "windows_x86_64_gnu"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"
+ [[package]]
+ name = "windows_x86_64_msvc"
+ version = "0.32.0"
+ source = "registry+https://github.com/rust-lang/crates.io-index"

Looks like linker can't find windows.lib. I'll keep digging further.

@ilslv
Copy link
Member Author

ilslv commented Mar 10, 2022

Ok, it looks like I've found the issue. windows-rs is including windows.lib via build script and then cargo carries this link to a native library with option like -L native=C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\windows_x86_64_msvc-0.32.0\lib. Unfortunately there is no way of getting that information while constructing rustc command inside skeptic crate itself. The only way to completely fix this is to rewrite skeptic to use cargo instead of rustc, which will lead to tests running slower. From what I can tell, trybuild uses exactly this approach.

For now I just used [path.crates-io] section to downgrade parking_lot to 0.11.

@ilslv
Copy link
Member Author

ilslv commented Mar 10, 2022

FCM

Work around linking error in book test on windows CI job (#208)

@ilslv ilslv added wontfix This will not be worked on and removed enhancement Improvement of existing features or bugfix labels Mar 10, 2022
@ilslv
Copy link
Member Author

ilslv commented Mar 10, 2022

Discussed: this is a known issue with skeptic crate. As possible solution is hacky and possibly will require us to ticker with it every once in a while, it's better to leave windows book test disabled on CI.

@ilslv ilslv closed this Mar 10, 2022
@tyranron tyranron deleted the fix-windows-link-error branch March 11, 2022 09:38
@tyranron tyranron added blocked Blocked for moving on by some condition and removed blocked Blocked for moving on by some condition labels Mar 11, 2022
@tyranron tyranron added this to the 0.12.1 milestone Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants