Fixing build for arm64 Windows #582

Honeybunch · 2022-05-27T21:44:30Z

A side effect of trying to get the arm64-android triplet available for ktx on vcpkg, I discovered that the arm64-windows build was also broken due to a compiler bug. So I've provided a workaround. See: microsoft/vcpkg#24869

Not sure if this is the best mechanism for applying a compiler flag globally to the astc-encoder but it did work. Happy to tweak this approach.

…into arm64-windows-fix

MarkCallow · 2022-05-29T10:01:50Z

Thank you for this. It is awesome that you are supporting ktx on vcpkg. I will report the compiler bug to ARM to get the workaround in the upstream astc-encoder.

Please modify the GitHub action to use the build script ci_scripts/build_win.ps1 which is used by the Appveyor build. I'd prefer to have all the Windows builds done via the same mechanisms. On Appveyor we have a matrix of environment variables set up to build the desired configurations. build_win.ps1 uses the values of these to set the cmake configuration. I'm not familiar with Actions but I expect something similar can be done.

The configuration should include FEATURE_DOC=ON, FEATURE_TOOLS=ON & SUPPORT_SSE=OFF. I'd also like to include FEATURE_JNI=ON if it can be done easily, i.e. if the GitHub Actions VM includes a JDK. (Appveyors' do). The Release build should also have PACKAGE=YES to build the installer.

Does Actions have a Windows arm64 VM available? If so it would be better to run the build on that so that KTX_FEATURE_TESTS can be on and the tests run after the CI build. As you have them off, you must have discovered that they error when cross-compiling. This is because cmake attempts to execute the compiled tests to have gtest add the list of tests to the cmake test runner. If the build must be done on x64 Windows then build_win.ps1 will have to be modified to support turning off the tests by adding FEATURE_TESTS to the list of environment variables it looks for and mapping that to KTX_FEATURE_TESTS in the cmake config call.

Honeybunch · 2022-05-29T16:27:23Z

Yeah I can update the yml to use the build script no problem

The only issue is that Github does not support arm64 on windows. There is no Server build of Windows available so they don't provided an arm64 windows env. See this closed PR for more details: actions/runner-images#768 . That's why I had to turn off the Tests. This is cross compiling with MSVC from an x64 host so the tests compile fine but they can't be run.

MarkCallow · 2022-05-30T01:35:23Z

The only issue is that Github does not support arm64 on windows.

Okay. Please modify the build script to support turning off the tests. If you want an example, ci_scripts/build_macos.sh is already doing this.

MarkCallow · 2022-05-30T14:11:42Z

I will very soon be landing changes for signing windows binaries and installers. I want to get this working on the arm build as well. For that I need equivalents to the following from upcoming .appveyor.yml changes to be added to the GitHub action:

In the global environment

WINDOWS_CODE_SIGN_IDENTITY: The Khronos Group Inc

In the install phase

- ps: iex ((New-Object Net.WebClient).DownloadString('https://raw.githubusercontent.com/appveyor/secure-file/master/install.ps1'))

In the before_build phase

# Install signing certificate.
- ps: |
    $cert_file = "the_khronos_group_inc.p12"
    appveyor-tools\secure-file -decrypt "${cert_file}.enc" -secret $env:CERT_DC_SECRET -salt $env:CERT_DC_SALT
    certutil -p $env:CERT_PK_PASS -importpfx My $cert_file NoExport
    rm $cert_file

The appveyor-tools for secure file decryption should work fine on any Windows system. The 3 environment variables are must be encrypted.

Please add these to this PR when you make the changes to use the script.

Honeybunch · 2022-05-30T21:56:30Z

So a couple notes from the first pass on this:

Github Action's windows-latest is only providing VS 2022 (I think) so as "windows-latest" changes the cmake generator declaration here will have to change too. Can't think of a way around this so far
The build directory name being specified as an ENV var can probably just be ignored. Each job in the matrix should have its own directory to work in. Also it seems to be complicated to tie two matrix vars together and I can't figure out a way to shortname the generators so that it works as a build dir name
The powershell script fails but Actions reports success because the script isn't returning a non-zero errno

…t will return failure codes

Honeybunch · 2022-05-30T23:04:08Z

Well the matrix was a good idea because packaging is broken right now. I reproduced this locally: a makensis call inside of cpack is failing. I think it's due to a syntax problem in cpack's generated code? The log made it look like it's trying to package the JNI code when it shouldn't but I made sure that was removed and it's still happening. Wild stuff. I'm attaching a copy of my log since I can't access the one generated on the build machine but I was able to repro the problem.
NSISOutput.log

Honeybunch · 2022-06-01T02:42:00Z

Just pulled master and that, as well as appveryor package just fine... Some of my changes must have broken something

Honeybunch · 2022-06-01T02:48:09Z

After some trial and error... you can't package a build that doesn't build tools. That makes sense...

… tools

Honeybunch · 2022-06-01T03:17:01Z

Don't know why that build failed downloading doxygen. I ran the action on my fork and it succeeded: https://github.com/Honeybunch/KTX-Software/actions/runs/2419034724

MarkCallow · 2022-06-01T09:46:02Z

Github Action's windows-latest is only providing VS 2022 (I think) so as "windows-latest" changes the cmake generator declaration here will have to change too. Can't think of a way around this so far

I don't entirely follow. Only VS2022 is fine. Is there some other problem?

The build directory name being specified as an ENV var can probably just be ignored. Each job in the matrix should have its own directory to work in. Also it seems to be complicated to tie two matrix vars together and I can't figure out a way to shortname the generators so that it works as a build dir name

I know nothing about Actions. On the other CI platforms I'm familiar with each job gets a pristine VM so they all can and do share a common build directory name. The purpose of the environment variable is to make sure the build directory name is passed to the deploy step, yet to be added here, without error. What is important is that any package name reflect the platform as all the packages will eventually end up in a single directory in GitHub Releases and people need to be able to pick their package.

I want to deploy the Windows ARM package and the Android package from Actions.

The powershell script fails but Actions reports success because the script isn't returning a non-zero errno

Sorry about that. This script returning an error is not generally all that useful because the VS builds it calls do not themselves return any error exit state. Nevertheless it is good to fix it.

MarkCallow

Please package only the release configuration to save build time.

.github/workflows/main.yml

MarkCallow · 2022-06-01T10:14:36Z

Don't know why that build failed downloading doxygen. I ran the action on my fork and it succeeded: https://github.com/Honeybunch/KTX-Software/actions/runs/2419034724

The installing doxygen step seems to have taken 0 seconds and there is no output. If you are installing Doxygen from Chocolatey there the problem was likely an issue communicating with the Chocolatey server. I use it from Appveyor for older build images and it is notoriously unreliable. Luckily VS2019 and later VM images on Appveyor come with Chocolately installed.

UPDATE: I clicked the broken check in the list below instead of the link in your comment. I see the error. I think all we can do is retry.

UPDATE 2: I've initiated a retry. Why are many of the windows-arm64 jobs after the broken one listed as dependents and therefore were not run? Do all the jobs share a VM in Actions so they need the Doxygen download to succeed?

We only need to include FEATURE_DOCS=YES in the Release config build we are packaging. The docs are the same in all configs.

MarkCallow · 2022-06-03T07:53:14Z

I have just landed the PR #583 for signing the Windows binaries and packages. Please rebase this PR with master to include it. You will have a conflict in ci_scripts/build_win.ps1 because #583 modifies one of the same lines you modified but it should be easy to resolve. Other than that there shouldn't be any problem.

…into arm64-windows-fix

Honeybunch · 2022-06-04T00:04:33Z

Just took care of the merge. Day job has been busy. Going to try to get this sorted this weekend

MarkCallow · 2022-06-04T09:30:10Z

I have tried to reproduce the Appveyor build failures and have failed. The same failures happen in master but are masked because, prior to your fix, build_win.ps1 always returns success. The failures are therefore are not related to your changes. I thought it might be related to the older version of cmake used by the VS2015 and VS2017 images but I've tried the same version on my Windows box together with VS2017 and could not reproduce the issue. I am out of time for today.

To pursue this I recommend

Eliminate from .appveyor.yml all but the VS2017 config that fails.
Remove the $ErrorActionPreference = 'Stop' from the build phase because the PowerShell that Appveyor uses truncates all but the last line of any error message emitted by a tool when this is set.
print the NSISOutputLog to the console so we can see it when looking at build log.

I also recommend cancelling all other builds after pushing these changes.

The mentioned variable WIN_CS_CERT_SEARCH_MACHINE_STORE is created as an option dependent on the presence of WIN_CODE_SIGN_IDENTITY and is only used in a single cmake if() command which should return false if it does not exist or is not set.

MarkCallow · 2022-06-11T10:22:17Z

Once PR #592 builds successfully I'll merge it to master

Done. Now lets get this PR finished.

…into arm64-windows-fix

MarkCallow

Some questions I'd like to know the answers to.

MarkCallow · 2022-06-12T04:50:42Z

lib/etcdec.cxx

@@ -174,6 +174,10 @@ submitted to the exclusive jurisdiction of the Swedish Courts.
 #if defined(_MSC_VER)


Can't you say

#if defined(_MSC_VER) && !__clang__

and then let the existing #elif __clang__ handle the clang case?

Done the way it is here raises the question for me of how does clang_cl handle the pragma warning statements. Does it just ignore them or does it warn about them.

I could do that yeah. An easy refactor for sure.
ClangCL does report MSVC's error codes so it does also handle the MSVC specific warning statements. That's why I did it this way, I figured whatever warnings we want to disable for MSVC we also want to disable for ClangCL.

It looks like you did refactor it.

If CLangCL is handling MSVC specific warnings there shouldn't be any need for the #pragma clang diagnostic ignored "-Wunused-parameter" as that is the same as warning 4100 in CL. Because we do need it, it looks like at best #pragma warning is being ignored. Is CLangCL's handling of #pragma warning documented somewhere?

I agree with the statement "I figured whatever warnings we want to disable for MSVC we also want to disable for ClangCL.". If the original way you did it implements this then restore it but add a comment as to why and about the way CLangCL handles these things.

Yes sorry looks like it just ignores them. This current approach works the same as it did before, it's no longer if/else with _MSC_VER so we apply msvc style warning pragmas for any compiler that has it defined. It's a little odd to read though so if you want I can make it explicit like I did before

The way you have it is fine but I suggest adding a comment // clang_cl defines both _MSC_VER and __clang__ to prevent somebody going down the road of trying #elif in future.

CMakeLists.txt

MarkCallow · 2022-06-13T06:47:00Z

CMakeLists.txt

+    # When targeting Windows arm64 CMAKE_SYSTEM_PROCESSOR will incorrectly 
+    # return AMD64.
+    # See: https://gitlab.kitware.com/cmake/cmake/-/issues/15170
+    # We assume that when builing for Windows arm64 that we are using MSVC


s/builing/building/

MarkCallow · 2022-06-13T06:51:59Z

CMakeLists.txt

@@ -524,7 +524,7 @@ PRIVATE
 #        "CMAKE_CXX_COMPILER_ID = ${CMAKE_CXX_COMPILER_ID} "
 #        "CMAKE_CXX_COMPILER_VERSION = ${CMAKE_CXX_COMPILER_VERSION}"
 #)
-if(MSVC)
+if(${CMAKE_CXX_COMPILER_ID} MATCHES "MSVC")


There is another if(MSVC) at line #158 that needs fixing. That is why the Appveyor CI build is failing.

Huh, not sure why Appveyor is failing yet but changing that at 158 causes other errors that make ClangCL builds fail with:
clang-cl : error : argument unused during compilation: '-O0'

Oh I see what's happening on appveyor. Yeah, strange. Let me see if I can figure out a better solution here

REALLY not sure why Appveyor's environment results in CMake not understanding the MSVC check

MarkCallow · 2022-06-13T07:31:18Z

lib/etcdec.cxx

-#elif __clang__
+  #pragma warning(disable: 4100 4244)
+#endif
+#if __clang__
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wunused-parameter"
  #if __clang_major__ > 13 || (__clang_major__ == 13 && __clang_minor__ >= 1)


We're still getting unused-but-set-variable warnings in the CLangCL builds. Probably the version number here needs adjusting. I think I must have found that version 13.0.0 complained that -Wunused-but-set-variable was unrecognized hence the carve out.

It appears that Apple Clang has different version numbers from Clang which is why CMake inrtroduced AppleCLang as a compiler id. Looking at this chart Apple Clang version 13.0.0 is based on clang 12.0.0. Not sure then why I got unused-but-set as unrecognized for 13.0.0 since we're seeing it in clang 12.0.0.

Anyway we need to fix version number handling here and in CMakeLists to account for the differences. According to the chart, Xcode 14 will have Apple clang 14.0.0 which is base on clang 14.0.0 so maybe they'll stay in better sync from here on.

Ahh! The fifth answer to this SO question says that in Clang you can use statements like #if __has_warning("-Wunused-but-set-variable"). That will be much better than version number heuristics. Please change this code accordingly.

Not sure what to do in CMakeLists.

MarkCallow · 2022-06-13T07:34:54Z

The mingw build is also getting warnings so these blocks of warning disables need to handle that compiler too. Isn't it GCC? It is better to make a new PR for fixing those and concentrate here on fixing clang-cl.

Honeybunch · 2022-06-13T23:56:25Z

Cleaned up some mingw warnings since I was curious and they were simple. Though I do see a warning with -Wclass-memaccess. I verified the mingw gcc compiler was invoked with the -Wno-class-memaccess flag already so I have no idea how to clean that one up

…ler at that version

CMakeLists.txt

MarkCallow · 2022-06-14T06:39:01Z

Though I do see a warning with -Wclass-memaccess. I verified the mingw gcc compiler was invoked with the -Wno-class-memaccess flag already so I have no idea how to clean that one up

Are you absolutely sure that -Wno-class-memaccess is being set when compiling lib/basisu/transcoder/basisu_transcoder.cpp? There are also 2 -Wmaybe-uninitialized warnings from the same file. -Wno-maybe-uninitialized is set in the same set_source_files_properties call for lib/basisu/transcoder/basisu_transcoder.cpp in CMakeLists.txt as -Wno-class-memaccess so it looks as if that set_source_files_properties at line 536 is not being called for some reason.

Honeybunch · 2022-06-14T23:34:43Z

I figured out where those warnings are coming from. They're from building the transcodetests target. I think it's because it references the path to the file differently so maybe it doesn't retain the same source file properties?

MarkCallow · 2022-06-15T09:23:14Z

Thank you @Honeybunch for this amazing piece of work with all its attention to detail.

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

Honeybunch added 3 commits May 27, 2022 14:39

Working around compiler bug when targeting arm64 windows

2966544

Merge branch 'master' of https://github.com/KhronosGroup/KTX-Software …

6daf650

…into arm64-windows-fix

Adding arm64 windows build to github actions

074db11

Honeybunch added 2 commits May 30, 2022 14:27

Making tests configurable in CI script for windows

6c3a69a

Trying out a matrix in github actions

2ac9944

Honeybunch added 7 commits May 30, 2022 14:56

Changing github actions VS generator string

9e469e4

Fixing some of the github action and making sure that the build scrip…

3320e2f

…t will return failure codes

Updating workflow to include doxygen

e9a72d1

Installing doxygen for real

939896d

Trying to fix syntax error in actions yml

ae00016

Trying to install doxygen...

725dbfe

I just want doxygen

6289afa

Honeybunch added 2 commits May 31, 2022 19:55

Adding error message if you try to package a build that doesn't build…

0fcf8de

… tools

Easiest build fix is to just tie packaging to building tools

3e0ee1e

MarkCallow requested changes Jun 1, 2022

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/KhronosGroup/KTX-Software …

4e32ebe

…into arm64-windows-fix

Honeybunch added 2 commits June 11, 2022 11:44

Merge branch 'master' of https://github.com/KhronosGroup/KTX-Software …

2c4594c

…into arm64-windows-fix

Fixing warnings with clang-cl

ed4a7c7

MarkCallow reviewed Jun 12, 2022

View reviewed changes

A more robust to muting clang warnings

cc4035c

MarkCallow reviewed Jun 13, 2022

View reviewed changes

Honeybunch added 5 commits June 13, 2022 16:10

Fixing comment typo

fa78044

Fixing bad msvc check in cmake

9d5e784

Fixing up some small issues

429d5f1

Trying a more thorough check for MSVC

5557739

Cleaning up some mingw warnings as a treat

41a6e1c

Honeybunch added 2 commits June 13, 2022 17:00

Lowering version of warning check to match observed behavior of compi…

2081778

…ler at that version

Trying to fix appveyor

9294717

MarkCallow reviewed Jun 14, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

Honeybunch added 2 commits June 14, 2022 16:38

Some cleanup and fixing some last warnings with mingw

f67090b

Tweak to approach in disabling warning

9fae48c

MarkCallow approved these changes Jun 15, 2022

View reviewed changes

MarkCallow merged commit b995ac3 into KhronosGroup:master Jun 15, 2022

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024

Fixing build for arm64 Windows (KhronosGroup#582)

0a5e97c

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fixing build for arm64 Windows (KhronosGroup#582)

6750b80

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fixing build for arm64 Windows (KhronosGroup#582)

78cda69

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fixing build for arm64 Windows (KhronosGroup#582)

e311778

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fixing build for arm64 Windows (KhronosGroup#582)

caa996a

* Set up code signing in GitHub actions. * Fix compile warnings in both clang_cl and mingw. * Fix build script to return failure codes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing build for arm64 Windows #582

Fixing build for arm64 Windows #582

Honeybunch commented May 27, 2022

MarkCallow commented May 29, 2022 •

edited

Loading

Honeybunch commented May 29, 2022 •

edited

Loading

MarkCallow commented May 30, 2022

MarkCallow commented May 30, 2022

Honeybunch commented May 30, 2022

Honeybunch commented May 30, 2022

Honeybunch commented Jun 1, 2022

Honeybunch commented Jun 1, 2022

Honeybunch commented Jun 1, 2022

MarkCallow commented Jun 1, 2022

MarkCallow left a comment

MarkCallow commented Jun 1, 2022 •

edited

Loading

MarkCallow commented Jun 3, 2022

Honeybunch commented Jun 4, 2022

MarkCallow commented Jun 4, 2022

MarkCallow commented Jun 11, 2022

MarkCallow left a comment

MarkCallow Jun 12, 2022

Honeybunch Jun 12, 2022

MarkCallow Jun 13, 2022

Honeybunch Jun 13, 2022

MarkCallow Jun 14, 2022

MarkCallow Jun 13, 2022

MarkCallow Jun 13, 2022

Honeybunch Jun 13, 2022

Honeybunch Jun 13, 2022

Honeybunch Jun 13, 2022

MarkCallow Jun 13, 2022 •

edited

Loading

MarkCallow commented Jun 13, 2022

Honeybunch commented Jun 13, 2022

MarkCallow commented Jun 14, 2022

Honeybunch commented Jun 14, 2022

MarkCallow commented Jun 15, 2022 •

edited

Loading

		@@ -174,6 +174,10 @@ submitted to the exclusive jurisdiction of the Swedish Courts.
		#if defined(_MSC_VER)

Fixing build for arm64 Windows #582

Fixing build for arm64 Windows #582

Conversation

Honeybunch commented May 27, 2022

MarkCallow commented May 29, 2022 • edited Loading

Honeybunch commented May 29, 2022 • edited Loading

MarkCallow commented May 30, 2022

MarkCallow commented May 30, 2022

Honeybunch commented May 30, 2022

Honeybunch commented May 30, 2022

Honeybunch commented Jun 1, 2022

Honeybunch commented Jun 1, 2022

Honeybunch commented Jun 1, 2022

MarkCallow commented Jun 1, 2022

MarkCallow left a comment

Choose a reason for hiding this comment

MarkCallow commented Jun 1, 2022 • edited Loading

MarkCallow commented Jun 3, 2022

Honeybunch commented Jun 4, 2022

MarkCallow commented Jun 4, 2022

MarkCallow commented Jun 11, 2022

MarkCallow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

MarkCallow commented Jun 13, 2022

Honeybunch commented Jun 13, 2022

MarkCallow commented Jun 14, 2022

Honeybunch commented Jun 14, 2022

MarkCallow commented Jun 15, 2022 • edited Loading

MarkCallow commented May 29, 2022 •

edited

Loading

Honeybunch commented May 29, 2022 •

edited

Loading

MarkCallow commented Jun 1, 2022 •

edited

Loading

MarkCallow Jun 13, 2022 •

edited

Loading

MarkCallow commented Jun 15, 2022 •

edited

Loading