[benchmark] Add MSVC ARM64 support

[janisozaur]

Adds MSVC ARM64 support to Google Benchmark.

PR sent upstream over week ago with no changes requested, but not merged yet and no information about planned releases.

[NancyLi1013]

Could you please also update CONTROL file?

Add Port-version field:

Port-Version: 1

[Neumann-A]

this is a failure in the vcpkg windows toolchain not setting CMAKE_SYSTEM_PROCESSOR to arm for ARM builds.

[janisozaur]

While vcpkg might share the issue, it's a consequence of MSVC's quirks and not limited to vcpkg. Even building manually with ninja requires this fixup.

[Neumann-A]

No it is a consequence of CMake always setting CMAKE_SYSTEM_PROCESSOR to CMAKE_HOST_SYSTEM_PROCESSOR (in make land this would be --build and not --host) unless otherwise specified. So if you crosscompile with CMake you are expected to correctly set the values for crosscompiling in the toolchain

[BillyONeal]

I think you need to let upstream discuss this google/benchmark#1052

[janisozaur]

There are two upstreams in this context:

• Gbenchmark, which is not very fond of this change due to cmake lacking relevant implementation or expressly stating this is the intended way of handling
• Cmake, which had the issue filed 6 years ago and no implementation in sight (though I haven't spent whole lot of time looking for one)

This leaves this PR in a limbo, as I don't intend to work on cmake fix at this point.

[BillyONeal]

@janisozaur My point is, we, vcpkg, are a packaging system, not a "fix everyone's bugs clearinghouse". If upstream looked at the patch and rejected it, it would be wrong of us to go around them and apply it anyway. Note that upstream is likely to be blamed for any bugs in such patches, not us. This isn't adding a header file or something like that, this is adding a whole new feature directly relevant to the upstream authors' core competencies. They probably have strong opinions about how timing stuff is done given that this is a benchmarking library. See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-implement-features-in-patches

As for the cmake change, I agree with other commenters that if that setting for CMAKE_CROSSCOMPILING needs to be set that way, it probably should be set everywhere in our toolchains, not in this individual port. But I don't think we even need to consider that right now because just the C++ patch is troublesome on its own.

[NancyLi1013]

@janisozaur

Since this PR requires upstream changes. We're not sure when upstream will complete these work.
I think we can close this PR for now. You can reopen and ping us if these changes are accepted by upstream.

What do you think?

[janisozaur]

As upstream had comments on the cmake part of the patch, not the actual code changes, I believe this is the correct way to address the compilation problem. The cmake stuff I've now separated out to another PR upstream, which I imagine can get rejected on the grounds that it's a problem with cmake and not something downstream should bother with. As vcpkg is a collection of cmake patches, I'd hope the CMakeLists.txt portion can get merged here until cmake itself gets fixed.

How does that sound to you?

[Neumann-A]

problem with cmake and not something downstream

let me just cite myself #14021 (comment)

It is a problem with vcpkg not setting CMAKE_SYSTEM_PROCESSOR on windows. The linux toolchain is actually setting those values.

[ras0219-msft]

I think we should wait on this until we merge the fixes to the Windows toolchain that @Neumann-A has implied.

[janisozaur]

Sounds ok. Is there pr already I can subscribe to?

[Neumann-A]

Sounds ok. Is there pr already I can subscribe to?

That is: #16111

[JackBoosY]

Since the required PR #16111 isn't merged, switch this PR to draft.

[janisozaur]

The upstream has merged both patches provided, so it should be possible to use it directly, without any patching. I will submit a pr later today

[janisozaur]

So I checked it just now on current master and it seems there's nothing to do anymore?

Upstream took two of my PRs which were both the gist of this PR to vcpkg:

google/benchmark#1090
google/benchmark#1052

Since they were merged, there was at least 1.5.5 release, which is the version currently in use by vcpkg:

vcpkg/ports/benchmark/portfile.cmake

Line 9 in 1345050

REF e991355c02b93fe17713efe04cbc2e278e00fdbd # v1.5.5

#18595

In 18595 arm was also removed from unsupported architectures and the scripts/ci.baseline.txt file was updated in #14246 to not include benchmark anymore.

It seems there are no changes required to get a build of benchmark working for WoA. Please confirm and close this PR or point what I might be missing.

[JackBoosY]

@janisozaur So can you please resolve the file conflicts first?

[JackBoosY]

So I think the upgrade PR #18595 fixed this issue?

[janisozaur]

Used current master to do a test build for arm64-windows and it compiled correctly:

https://github.com/janisozaur/benchmark-arm64-test/runs/3158745801?check_suite_focus=true

https://github.com/janisozaur/benchmark-arm64-test/blob/main/.github/workflows/ci.yml

name: CI
on: [push, pull_request]
jobs:
  windows:
    name: Windows
    runs-on: windows-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v2
      - name: Install vcpkg and packages
        uses: lukka/run-vcpkg@v7
        id: runvcpkg
        with:
          vcpkgGitCommitId: d781bd9ca77ac3dc2f13d88169021d48459c665f
          vcpkgTriplet: 'arm64-windows'
          vcpkgArguments: 'benchmark'

I can't test it right now, but given all the issues this PR aimed to fix were spotted at compile-time, I believe this PR is no longer required as upstream took my fixes there.