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 the static library might failed to link on Windows #251

Merged

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Apr 14, 2023

Motivation

Currently the CI in master is broken, here is an example failure: https://github.com/apache/pulsar-client-cpp/actions/runs/4688588681/jobs/8309495018?pr=249

The reason is that the latest Windows runner image is already integrated with the OpenSSL library to link, i.e. the COMMON_LIBS list variable might have a sub-list like:

debug;C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib

The list above have two elements:

  1. debug
  2. C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib

When building the static library, the list above will be converted to a space-separated string like:

C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib

i.e. the debug and optimized elements are removed, the rest elements that follows debug or optimized (determined by the build mode) will be retained.

See

string(REPLACE ";" " " TEMP_OUT "${LIBLIST}")

However, since there is a blank in Program Files, the string above is unexpectedly treated as two elements:

  • C:/Program
  • Files/OpenSSL/lib/VC/libcrypto64MDd.lib

Then, the CI failed when linking to pulsarWithDeps.lib:

fatal error LNK1181: cannot open input file 'C:\Program'

Modifications

Instead of setting the STATIC_LIBRARY_FLAGS property, set the STATIC_LIBRARY_OPTIONS property, which is a list rather than a string. So the blank in the list element won't affect. Then in remove_libtype, return a list instead of a string.

References:

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower BewareMyPower added the bug Something isn't working label Apr 14, 2023
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Apr 14, 2023
@BewareMyPower BewareMyPower self-assigned this Apr 14, 2023
@BewareMyPower BewareMyPower marked this pull request as draft April 14, 2023 05:28
### Motivation

Currently the CI in master is broken, here is an example failure:
https://github.com/apache/pulsar-client-cpp/actions/runs/4688588681/jobs/8309495018?pr=249

The reason is that the latest Windows runner image is already integrated
with the OpenSSL library to link, i.e. the `COMMON_LIBS` list variable
might have an element like:

```
debug;C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib
```

The list above have two elements:
1. `debug`
2. `C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib`

When building the static library, the list above will be converted to a
space-separated string like:

```
C:/Program Files/OpenSSL/lib/VC/libcrypto64MDd.lib
```

i.e. the `debug` and `optimized` elements are removed, the rest elements
that follows `debug` or `optimized` (determined by the build mode) will
be retained.

See https://github.com/apache/pulsar-client-cpp/blob/a57bb072ce6140757917353cc1d5a0007ddc353d/lib/CMakeLists.txt#L109

However, since there is a blank in `Program Files`, the string above is
unexpectedly treated as two elements:
- `C:/Program`
- `Files/OpenSSL/lib/VC/libcrypto64MDd.lib`

Then, the CI failed when linking to `pulsarWithDeps.lib`:

> fatal error LNK1181: cannot open input file 'C:\Program'

### Modifications

Instead of setting the `STATIC_LIBRARY_FLAGS` property, set the
`STATIC_LIBRARY_OPTIONS` property, which is a list rather than a string.
So the blank in the list element won't affect. Then in `remove_libtype`,
return a list instead of a string.

References:
- https://cmake.org/cmake/help/latest/prop_tgt/STATIC_LIBRARY_OPTIONS.html#prop_tgt:STATIC_LIBRARY_OPTIONS
- https://cmake.org/cmake/help/latest/prop_tgt/STATIC_LIBRARY_FLAGS.html#prop_tgt:STATIC_LIBRARY_FLAGS
@BewareMyPower BewareMyPower force-pushed the bewaremypower/win64-debug-build branch from ca9fa06 to 6dfeabf Compare April 14, 2023 05:43
@BewareMyPower BewareMyPower marked this pull request as ready for review April 14, 2023 05:44
@BewareMyPower BewareMyPower merged commit 713e254 into apache:main Apr 14, 2023
@BewareMyPower BewareMyPower deleted the bewaremypower/win64-debug-build branch April 14, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants