[spdlog] Fix the non-Windows build issue by scoping the define to Windows targets.

[kromain]

Describe the pull request

• What does your PR fix?

  The previous portfile update (840f701d83d5019aa5033c9d) broke non-Windows builds when the wchar feature is enabled. The feature is allowed for non-Windows systems, just getting ignored in that case. But the change that adds the #define SPDLOG_WCHAR_TO_UTF8_SUPPORT is not scoped to Windows, and having that define in a non-Windows build triggers an #error:

build/vcpkg_installed/x64-linux/include/spdlog/common.h:182:10: error: #error SPDLOG_WCHAR_TO_UTF8_SUPPORT only supported on windows

This PR fixes the non-Windows build issue by scoping the define to Windows targets.

• Which triplets are supported/not supported? Have you updated the CI baseline?

  all, No

• Does your PR follow the maintainer guide?

  Yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

  Yes

[kromain]

CC @diablodale

[diablodale]

good catch, logic makes sense.
It is curious why the CI didn't catch this bug when I made the PR #24830
Some of the logs are not available, but I can see on the 'checks' tab, azure pipelines, that the osx and linux runs passed.
This suggests to me that the CI is failing to catch bugs like this. 🤔

[autoantwort]

The better fix would be to add an supports expression to the feature :)

[diablodale]

The better fix would be to add an supports expression to the feature :)

Code snippet?

[autoantwort]

--- a/ports/spdlog/vcpkg.json
+++ b/ports/spdlog/vcpkg.json
@@ -24,7 +24,8 @@
       ]
     },
     "wchar": {
-      "description": "Build with wchar_t (Windows only)"
+      "description": "Build with wchar_t (Windows only)",
+      "supports": "windows"
     }
   }
 }

[kromain]

Thanks for the tip! Looks simpler too. will update the PR shortly

[diablodale]

@JonLiu1993 does someone on your team understand why CI didn't catch this bug on linux, osx, etc.?

[kromain]

after trying @autoantwort's patch locally, one downside of using "supports": "windows" is that it breaks vcpkg install on non-windows platforms if the feature is enabled for the dependency:

1> [CMake] -- Running vcpkg install
1> [CMake] Error: spdlog:x64-linux@1.10.0#2 The feature wchar is only supported on 'windows'

This forces users to split their dependency definition in two if they're targeting multiple platforms, which feels a little clunky tbh:

    {
      "name": "spdlog",
      "features": [ "wchar" ],
      "platform": "windows"
    },
    {
      "name": "spdlog",
      "platform": "!windows"
    }

I still appreciate that the intent of supports is exactly for what I'm trying to fix, I just wish it would print a warning, like the current fix, rather than flat-out error and fail the vcpkg install step.

I'm undecided on which fix is better: the first fix that's backwards-compatible, or the supports fix that's more "language-correct" but also makes things harder for multi-platform situations. What are the reviewers' preferences?

[autoantwort]

You can use --allow-unsupported

[kromain]

@JonLiu1993 does someone on your team understand why CI didn't catch this bug on linux, osx, etc.?

looking at the logs from the checks, I think the reason it didn't catch it is that the test builds don't specify additional features, they just build with default features (which in this case wouldn't include wchar).

[autoantwort]

Maybe vcpkg should just skip features that are not supported for a triplet, but that can lead to surprising results as well.
Or add a field "skip-unsupported-features": true or do

{
   "features": [{"name": "wchar", "platform": "windows"}]
}

[kromain]

 {
    "features": [{"name": "wchar", "platform": "windows"}]
 }

Yes that would be my preferred workaround. I'm afraid passing a blanket --allow-unsupported to the whole command could have other unintended consequences.

Anyway, I can live with adding the separate dependency to avoid the error from supports. Thanks for mentioning the argument though, that means we should keep the changes in the portfile regardless since the error can be suppressed.

[diablodale]

@JonLiu1993 does someone on your team understand why CI didn't catch this bug on linux, osx, etc.?

looking at the logs from the checks, I think the reason it didn't catch it is that the test builds don't specify additional features, they just build with default features (which in this case wouldn't include wchar).

Hmmm. Is testing only the default features of a port a vcpkg-wide rule? Or is this something specific to this port?
I ask because the former is a rule for the core team to own while the latter is a bug (or lack of feature) for this specific port.

[autoantwort]

Is testing only the default features of a port a vcpkg-wide rule?

Yes

[JonLiu1993]

@JonLiu1993 does someone on your team understand why CI didn't catch this bug on linux, osx, etc.?

@kromain, ci will only test the default features. We need to manually test other features locally. the wchar feature will not be tested on ci, so no error is reported in ci test, In principle, we need to test these features locally.

[dg0yt]

FTR there are examples of features which are documented to do nothing in certain contexts. So this approach can be acceptable but then it it should really do nothing ;-)

With an explicit supports guard, it can be used a little bit simpler:

    "spdlog",
    {
      "name": "spdlog",
      "features": [ "wchar" ],
      "platform": "windows"
    }

(But it gets complicated again as soon as you want/need to add "default-feautures": false.)

[JonLiu1993]

@kromain, When I tested the features by command "./vcpkg install spdlog[*]:x86-windows" I got the error:

F:\Feature-test\spdlog\vcpkg\buildtrees\spdlog\src\v1.10.0-3cbe543323.clean\bench\bench.cpp(18): fatal error C1083: Cannot open include file: 'fmt/locale.h': No such file or directory`

Please take a look.

[kromain]

Indeed, I see the same error when using spdlog[*], but it's unrelated to my changes. The same happens on master.

I've tracked it down to the benchmark feature that broke following the update of fmt to the 9.0.0 release, which removed the deprecated fmt/locale.h (see release notes).

There was actually a long-standing warning in that header to include a different file, but I guess nobody noticed it on the spdlog side. It's a trivial fix, I can submit a patch to spdlog and wait for the update to the next release, or if you think it's critical enough we can also add the patch to the port in the meantime.

In any case, I think it should be addressed in a separate PR as it's specific to the benchmark feature.

[JonLiu1993]

Feature wchar tested successfully in the following triplet:

• x86-windows
• x64-windows
• x64-windows-static

[dg0yt]

PR title:

Fix non-Windows builds with wchar feature enabled

Review:

Feature wchar tested successfully in the following triplet:

• x86-windows
• x64-windows
• x64-windows-static

Find the problem!

[kromain]

FYI the fix for the issue with benchmark has been merged into the spdlog master branch: gabime/spdlog#2545

[BillyONeal]

@kromain

one downside of using "supports": "windows" is that it breaks vcpkg install on non-windows platforms if the feature is enabled for the dependency:

You say downside, I would argue this is an upside. If someone explicitly asks for wchar_t and that can't be provided, the right behavior is to tell them that doesn't work, not ignore what they asked for.

I think it would also be reasonable to remove the feature entirely, and just make the choice that this is on for Windows and off for everyone else. Then the user hasn't made an explicit request we need to honor or fail for.

Thanks for the fix!

---

@dg0yt

PR title:

Fix non-Windows builds with wchar feature enabled

Review:

Feature wchar tested successfully in the following triplet:

• x86-windows
• x64-windows
• x64-windows-static

Find the problem!

The PR makes building the feature on non-Windows triplets impossible, so I'm not sure what you would expect @JonLiu1993 to have done here. He checked that Windows didn't regress given that CI doesn't check that for us automatically.

---

@JonLiu1993 Thanks for checking that things still work!

[BillyONeal]

I'm weakly against defending against unsupported configurations like this but I don't think it's that "harmful". I would not expect future edits to this port to preserve this behavior, for instance.