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.

Sign up for GitHub

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

Jump to bottom

Linking fails for library name contains .dll suffix #19696

Closed
Felix-El opened this issue Oct 2, 2023 · 1 comment · May be fixed by #19697
Closed

Linking fails for library name contains .dll suffix #19696

Felix-El opened this issue Oct 2, 2023 · 1 comment · May be fixed by #19697
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@Felix-El
Copy link

Felix-El commented Oct 2, 2023 •
edited
Loading

Description of the bug:

While learning how to define a custom Bazel toolchain, I decided to use mingw64 to cross compile on Linux for Windows.
I've created a simple project (link below -> reproduction) with a shared library and an executable that I'm building using that toolchain. This worked fine when building for Linux, with auto-detected toolchain but cross-compiling for Windows, I got linker errors.

Linking the executable fails because the command line contains -lmylib.dll whereas -lmylib is expected. After hours of digging I've found this piece of code:

    boolean hasCompatibleName =
        name.startsWith("lib") || (!name.endsWith(".so") && !name.endsWith(".dylib"));
    if (CppFileTypes.SHARED_LIBRARY.matches(name) && hasCompatibleName) {
      String libName = name.replaceAll("(^lib|\\.(so|dylib)$)", "");
      librariesToLink.addValue(LibraryToLinkValue.forDynamicLibrary(libName));
    } else if (CppFileTypes.SHARED_LIBRARY.matches(name)
        || CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(name)) {
      librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name));
    } else {
      // Interface shared objects have a non-standard extension
      // that the linker won't be able to find.  So use the
      // filename directly rather than a -l option.  Since the
      // library has an SONAME attribute, this will work fine.
      librariesToLink.addValue(
          LibraryToLinkValue.forInterfaceLibrary(inputArtifact.getExecPathString()));
    }

On Linux, the ".so" suffix is correctly stripped, on Windows it is not.
Adding "|dll" to the replaceAll regular expression fixed the issue for me.
I would assume to properly fix this, one would also add && !name.endsWith(".dll") for hasCompatibleName.

Which category does this issue belong to?

C++/Objective-C Rules, Configurability, Core

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Build my learning MinGW64 project, according README.md.

Which operating system are you running Bazel on?

Ubuntu 22.04.3 LTS

What is the output of bazel info release?

development version

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

Clone official master (at dd30503), use existing bazel(isk) to build //src:bazel-dev.

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

https://github.com/bazelbuild/bazel.git
dd305035b031081423ab51041ee525d38236a737
dd305035b031081423ab51041ee525d38236a737

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response
@Felix-El Felix-El mentioned this issue Oct 2, 2023
Open
@iancha1992 iancha1992 added the team-Rules-CPP Issues for C++ rules label Oct 2, 2023
@iancha1992 iancha1992 added the team-Rules-ObjC Issues for Objective-C maintainers label Oct 2, 2023
@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged team-Rules-ObjC Issues for Objective-C maintainers labels Nov 27, 2023
This was referenced Feb 9, 2024
Closed
Closed
@vvviktor vvviktor mentioned this issue Feb 19, 2024
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Feb 29, 2024
@vvviktor @bazel-io
Update LibrariesToLinkCollector.java for .dll suffix stripping
b4c86fa 
Fixes bazelbuild#19696

Allow automatic linkage of DLL libraries when GNU toolchain used in Windows. .dll suffix has to be stripped before passing library name to `ld.exe` with `-l` option.

This [case](https://github.com/vvviktor/bazel_sandbox.git) was successfully tested :

Workspace structure:
```
D:.
│   .bazelrc
│   BUILD
│   MODULE.bazel
│   MODULE.bazel.lock
│   WORKSPACE
│
├───Main
│       BUILD
│       main.cpp
│       math.cpp
│       math.h
│       math_dll_interface.cpp
│       math_dll_interface.h
│       math_import_defs.h
│
└───toolchain
        BUILD
        toolchain_config.bzl
```

BUILD file:
```
# //Main/BUILD

DLL_HDRS = ["math_import_defs.h", "math_dll_interface.h"]

cc_binary(
    name = "sum_numbers_mingw",
    srcs = ["main.cpp"],
    deps = [":math_d_shared"]
)

cc_import(
    name = "math_d_shared",
    hdrs = DLL_HDRS,
    shared_library = ":libmath_d.dll"
)

cc_binary(
    name = "libmath_d.dll",
    srcs = ["math_dll_interface.cpp"] + DLL_HDRS,
    deps = [":math"],
    defines = ["MATH_DLL"],
    linkshared = 1
)

cc_library(
    name = "math",
    srcs = ["math.cpp"],
    hdrs = ["math.h"],
    copts = ["-std=c++17"]
)
```
Without patch applied `bazel build //main:sum_numbers_mingw --verbose_failures` fails with error:
`C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: cannot find -lmath_d.dll: No such file or directory`
Then after patch was applied it builds all targets as expected.

This approach also works fine with patch applied:
```
# //Main/BUILD

DLL_HDRS = ["math_import_defs.h", "math_dll_interface.h"]

cc_binary(
    name = "sum_numbers_mingw",
    srcs = ["main.cpp"] + DLL_HDRS,
    dynamic_deps = [":math_d_shared"]
)

cc_shared_library(
    name = "math_d_shared",
    shared_lib_name = "libmath_d.dll",
    deps = [":math_dll_interface"]
)

cc_library(
    name = "math_dll_interface",
    srcs = ["math_dll_interface.cpp"],
    hdrs = DLL_HDRS,
    deps = [":math"],
    defines = ["MATH_DLL"]
)

cc_library(
    name = "math",
    srcs = ["math.cpp"],
    hdrs = ["math.h"],
    copts = ["-std=c++17"]
)
```

Closes bazelbuild#21404.

PiperOrigin-RevId: 611401823
Change-Id: I98fbfb245acdd2dac41d6a56b5f74059dc53a082
@bazel-io bazel-io mentioned this issue Feb 29, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 4, 2024
@bazel-io @vvviktor @meteorcloudy
[7.1.0] Update LibrariesToLinkCollector.java for .dll suffix stripping (
52443d0 
#21524)

Fixes #19696

Allow automatic linkage of DLL libraries when GNU toolchain used in
Windows. .dll suffix has to be stripped before passing library name to
`ld.exe` with `-l` option.

This [case](https://github.com/vvviktor/bazel_sandbox.git) was
successfully tested :

Workspace structure:
```
D:.
│   .bazelrc
│   BUILD
│   MODULE.bazel
│   MODULE.bazel.lock
│   WORKSPACE
│
├───Main
│       BUILD
│       main.cpp
│       math.cpp
│       math.h
│       math_dll_interface.cpp
│       math_dll_interface.h
│       math_import_defs.h
│
└───toolchain
        BUILD
        toolchain_config.bzl
```

BUILD file:
```
# //Main/BUILD

DLL_HDRS = ["math_import_defs.h", "math_dll_interface.h"]

cc_binary(
    name = "sum_numbers_mingw",
    srcs = ["main.cpp"],
    deps = [":math_d_shared"]
)

cc_import(
    name = "math_d_shared",
    hdrs = DLL_HDRS,
    shared_library = ":libmath_d.dll"
)

cc_binary(
    name = "libmath_d.dll",
    srcs = ["math_dll_interface.cpp"] + DLL_HDRS,
    deps = [":math"],
    defines = ["MATH_DLL"],
    linkshared = 1
)

cc_library(
    name = "math",
    srcs = ["math.cpp"],
    hdrs = ["math.h"],
    copts = ["-std=c++17"]
)
```
Without patch applied `bazel build //main:sum_numbers_mingw
--verbose_failures` fails with error:

`C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe:
cannot find -lmath_d.dll: No such file or directory`
Then after patch was applied it builds all targets as expected.

This approach also works fine with patch applied:
```
# //Main/BUILD

DLL_HDRS = ["math_import_defs.h", "math_dll_interface.h"]

cc_binary(
    name = "sum_numbers_mingw",
    srcs = ["main.cpp"] + DLL_HDRS,
    dynamic_deps = [":math_d_shared"]
)

cc_shared_library(
    name = "math_d_shared",
    shared_lib_name = "libmath_d.dll",
    deps = [":math_dll_interface"]
)

cc_library(
    name = "math_dll_interface",
    srcs = ["math_dll_interface.cpp"],
    hdrs = DLL_HDRS,
    deps = [":math"],
    defines = ["MATH_DLL"]
)

cc_library(
    name = "math",
    srcs = ["math.cpp"],
    hdrs = ["math.h"],
    copts = ["-std=c++17"]
)
```

Closes #21404.

Commit
e2837db

PiperOrigin-RevId: 611401823
Change-Id: I98fbfb245acdd2dac41d6a56b5f74059dc53a082

Co-authored-by: Viktor Kustov <[email protected]>
Co-authored-by: Yun Peng <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.1.0 RC2. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.1.0rc2. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Milestone
No milestone
5 participants
@comius @sgowroji @Felix-El @Pavank1992 @iancha1992