Skip to content

New linker error when building diesel with rust-beta on windows #139352

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

Closed
weiznich opened this issue Apr 4, 2025 · 11 comments
Closed

New linker error when building diesel with rust-beta on windows #139352

weiznich opened this issue Apr 4, 2025 · 11 comments
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@weiznich
Copy link
Contributor

weiznich commented Apr 4, 2025

Code

I tried this code: https://github.com./diesel-rs/diesel/tree/2.2.x (2464571)

I'm using the following command to execute tests: cargo +beta test --manifest-path diesel_derives/Cargo.toml --no-default-features --features "diesel/mysql mysql"

I expected to see this happen: Code compiles successful and tests complete (Link to the relevant CI run)

Instead, this happened: Code fails to compile with a linking error on windows (other systems work fine):

  = note:    Creating library D:\a\diesel\diesel\target\debug\deps\tests-1cc142ef763bf67a.lib and object D:\a\diesel\diesel\target\debug\deps\tests-1cc142ef763bf67a.exp␍
          mysqlclient.lib(my_init.obj) : error LNK2019: unresolved external symbol __imp_RegCloseKey referenced in function "bool __cdecl win32_have_tcpip(void)" (?win32_have_tcpip@@YA_NXZ)␍
          mysqlclient.lib(my_init.obj) : error LNK2019: unresolved external symbol __imp_RegEnumValueA referenced in function "void __cdecl win_init_registry(void)" (?win_init_registry@@YAXXZ)␍
          mysqlclient.lib(my_init.obj) : error LNK2019: unresolved external symbol __imp_RegOpenKeyExA referenced in function "bool __cdecl win32_have_tcpip(void)" (?win32_have_tcpip@@YA_NXZ)␍
          mysqlclient.lib(common.obj) : error LNK2019: unresolved external symbol __imp_EqualSid referenced in function "public: bool __cdecl Sid::operator==(class Sid const &)" (??8Sid@@QEAA_NAEBV0@@Z)␍
          mysqlclient.lib(common.obj) : error LNK2019: unresolved external symbol __imp_GetTokenInformation referenced in function "public: __cdecl Sid::Sid(void *)" (??0Sid@@QEAA@PEAX@Z)␍
          mysqlclient.lib(common.obj) : error LNK2019: unresolved external symbol __imp_IsValidSid referenced in function "public: bool __cdecl Sid::is_valid(void)const " (?is_valid@Sid@@QEBA_NXZ)␍
          mysqlclient.lib(common.obj) : error LNK2019: unresolved external symbol __imp_LookupAccountNameW referenced in function "public: __cdecl Sid::Sid(wchar_t const *)" (??0Sid@@QEAA@PEB_W@Z)␍
          D:\a\diesel\diesel\target\debug\deps\tests-1cc142ef763bf67a.exe : fatal error LNK1120: 7 unresolved externals␍

Link to the relevant CI run

Version it worked on

It most recently worked on: Current stable (1.86.0)

Version with regression

rustc --version --verbose:

rustc 1.87.0-beta.1 (45165c82a 2025-04-01)

@rustbot label: O-windows regression-from-stable-to-beta

@weiznich weiznich added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Apr 4, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. O-windows Operating system: Windows regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 4, 2025
@tgross35 tgross35 added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com./rust-lang/cargo-bisect-rustc label Apr 4, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Apr 4, 2025

Judging from symbol names, probably #138233 (relnotes #138621) but someone more familiar with Windows might want to double-check.

@jieyouxu jieyouxu added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 4, 2025
@apiraino
Copy link
Contributor

apiraino commented Apr 4, 2025

Yes, @weiznich noted that in comment and patched a fix in sgrif/mysqlclient-sys@2b3ff91

(cross-linking FIY, assuming this issue is about that)

@weiznich
Copy link
Contributor Author

weiznich commented Apr 4, 2025

Just as a heads up: The linked fix does not completely fix the problem due to mistakes on my side. Additionally it only fixes users that actively update to a newer version. Any older mysqlclient-sys version is still affected by this build breaking change.

@oli-obk oli-obk added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Apr 4, 2025
@tgross35 tgross35 removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com./rust-lang/cargo-bisect-rustc label Apr 4, 2025
@apiraino
Copy link
Contributor

apiraino commented Apr 7, 2025

I think cc: @smmalis37 @ChrisDenton for #138233

@ChrisDenton
Copy link
Member

We could delay the change to give people more time to update but in general crates cannot depend on implementation details of the standard library if they want stability (as most do).

@weiznich
Copy link
Contributor Author

weiznich commented Apr 7, 2025

I can understand how that is considered to be a unstable implementation detail. What's really hard for me as crate author to find out what is such an implementation detail and what not and how to check if I depend on such a detail. So it would be super helpful to have a clear documentation on what's such a implementation detail and how to check if your crate depends on some internal detail. Ideally as some sort of tool/lint or similar. That must not even check for everything at once, but just something that gives you a release cycle or two of warning period that says: This will break soon because it's not considered public API. The problem for me is that it is quite stressful to find out one day that something is broken for (valid) reasons again.

@ChrisDenton
Copy link
Member

The problem here is in C a dependency so I'm not sure a lint would help. It doesn't declare the import libraries it depends on as far as I can tell so even changes to it could add an undeclared dependency and therefore break your build. Interestingly some of its "extra" packages do it properly (i.e. https://github.com./search?q=repo%3Aweiznich%2Fmysql-server%20advapi32&type=code) so maybe they rely on that for their own builds. I don't know.

In Rust this specific case can be tested for by building a binary that invokes a linker (so an executable or dll) when the standard library is built with the windows_raw_dylib feature. This is unstable and requires nightly:

cargo +nightly build -Z build-std=core,alloc,std -Z build-std-features=backtrace,panic-unwind,windows_raw_dylib

A no-std build that uses the C libraries generated in your build script would be more robust but perhaps harder to implement.

@Amanieu
Copy link
Member

Amanieu commented Apr 9, 2025

We discussed this in the @rust-lang/libs meeting today and concluded that, because this usually happens deep in dependency trees, delaying this change wouldn't help since most users would only notice this once their build actually fails. The compat notes in #138621 should simply advise users to upgrade their dependencies if they encounter this problem.

@Amanieu Amanieu closed this as completed Apr 9, 2025
@weiznich
Copy link
Contributor Author

I agree with the assessment that it would not be helpful to delay releasing this change.

What I still would like to see is the documentation mentioned above. I believe that's something that should be in place for the release so that the compat notes can easily link there and state: By the way that are the dependencies we guarantee to link and those are currently linked, but not guaranteed to be there in the future. You can test for future incompatibilities like that (command from above).

Given that this doesn't exist yet I personally do not consider this to be completed and would like to request this issue to be reopened (cc @Amanieu which closed the issue).

@oli-obk
Copy link
Contributor

oli-obk commented Apr 10, 2025

This issue (handling the new linker error) is resolved. Please open a separate issue for adding additional documentation

@apiraino apiraino removed E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Apr 10, 2025
@weiznich
Copy link
Contributor Author

I filled #139619 for the additional documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants