Skip to content

Use "raw-dylib" for non-win7 Windows Targets #627

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

Merged
merged 1 commit into from
Mar 9, 2025
Merged

Use "raw-dylib" for non-win7 Windows Targets #627

merged 1 commit into from
Mar 9, 2025

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Mar 9, 2025

Now that we only use the standard Windows backend on Windows 1.78 or later, we can just use kind = "raw-dylib" to link to bcryptprimitives.dll.

This eliminates the windows-targets dependency and reduces the --target=all dependancy graph for getrandom to:

└── getrandom v0.3.1
    ├── cfg-if v1.0.0
    ├── libc v0.2.170
    ├── r-efi v5.2.0
    └── wasi v0.14.2+wasi-0.2.4
        └── wit-bindgen-rt v0.39.0
            └── bitflags v2.9.0

@josephlr josephlr requested a review from newpavlov March 9, 2025 01:24
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Could you also add a changelog entry for this PR?

Now that we only use the standard Windows backend on Windows 1.78 or
later, we can just use `kind = "raw-dylib"` to link to
`bcryptprimitives.dll`.

This reduces the `--target=all` dependancy graph for `getrandom` to:
```
└── getrandom v0.3.1
    ├── cfg-if v1.0.0
    ├── libc v0.2.170
    ├── r-efi v5.2.0
    └── wasi v0.14.2+wasi-0.2.4
        └── wit-bindgen-rt v0.39.0
            └── bitflags v2.9.0
```

Signed-off-by: Joe Richey <[email protected]>
@josephlr
Copy link
Member Author

josephlr commented Mar 9, 2025

Could you also add a changelog entry for this PR?

Done, and removed the note about #593 as the dependency is now fully removed.

@josephlr josephlr merged commit b160ef6 into master Mar 9, 2025
71 checks passed
@josephlr josephlr deleted the raw_dylib branch March 9, 2025 03:53
@newpavlov newpavlov mentioned this pull request Mar 10, 2025
newpavlov added a commit that referenced this pull request Mar 17, 2025
### Added
- `efi_rng` opt-in backend [#570]
- `linux_raw` opt-in backend [#572]
- `.cargo/config.toml` example in the crate-level docs [#591]
- `getrandom_test_linux_without_fallback` configuration flag to test
that file fallback
  is not triggered in the `linux_android_with_fallback` backend [#605]
- Built-in support for `*-linux-none` targets [#618]
- Cygwin support [#626]

### Changed
- Update `wasi` dependency to v0.14 [#594]
- Add `#[inline]` attribute to the inner functions [#596]
- Update WASI and Emscripten links in the crate-level docs [#597]
- Do not use `dlsym` on MUSL targets in the
`linux_android_with_fallback` backend [#602]
- Remove `linux_android.rs` and use `getrandom.rs` instead [#603]
- Always use `RtlGenRandom` on Windows targets when compiling with
pre-1.78 Rust [#610]
- Internal representation of the `Error` type [#614]
- Remove `windows-targets` dependency and use [`raw-dylib`] directly
[#627]

### Removed
- `Error::INTERNAL_START` and `Error::CUSTOM_START` associated constants
[#614]

[#570]: #570
[#572]: #572
[#591]: #591
[#594]: #594
[#596]: #596
[#597]: #597
[#602]: #602
[#603]: #603
[#605]: #605
[#610]: #610
[#614]: #614
[#618]: #618
[#626]: #626
[#627]: #627
[`raw-dylib`]:
https://doc.rust-lang.org/reference/items/external-blocks.html?highlight=link#dylib-versus-raw-dylib
@glandium
Copy link

glandium commented Mar 25, 2025

Unfortunately, there are issues with raw-dylibs before 1.84.0 on i686-pc-windows-msvc, and still to this day on i686-pc-windows-gnu. See rust-lang/rust#130586

@newpavlov
Copy link
Member

newpavlov commented Mar 25, 2025

@glandium
Does it mean that getrandom fails to compile on i686-pc-windows-gnu with Rust 1.78-1.84 and on other Windows targets with Rust 1.78-1.83? Could you create a new issue for that with more information?

We don't see any issues as part of our CI jobs.

@glandium
Copy link

glandium commented Mar 26, 2025

The issue is actually hard to reproduce in a visible way. I haven't managed to reproduce it outside the Firefox build so far. It requires several stars being aligned, the minimum of which is:

  • raw-dylib
  • crate-type = staticlib
  • linking with something else that also uses the symbol with lld
  • delayload the library containing the symbol
    and I'm not sure what else because trying to come up with a test case with only the above doesn't reproduce the issue (I can observe something weird, though, that could be related). But I can totally reproduce in the Firefox build.
    This leads to ldd complaining about duplicate symbols for __load_$symbol on i686-pc-windows-{gnu,msvc}, where the issue was fixed in 1.84.0 for i686-pc-windows-msvc, but still exists with i686-pc-windows-gnu.

So, I'm not surprised you don't see any issues on your CI.

Edit: I opened rust-lang/rust#138963

@newpavlov
Copy link
Member

newpavlov commented Mar 26, 2025

As a potential workaround we could expose the "windows legacy" backend as an opt-in backend. Would it work for you? (UPD: we technically already have the getrandom_windows_legacy flag, but it's more of an implementation detail)

We also could modify our build.rs to account for this bug, i.e. switch to the "legacy" backend on pre-1.84 compilers and on i686 targets until the bug is fixed. But personally I would prefer to not introduce such workarounds for somewhat exotic compiler bugs (the 1.84 part is fine, but the target-dependent part is more questionable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants