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: build on Windows ARM64 #913

Merged
merged 1 commit into from
Aug 4, 2023
Merged

fix: build on Windows ARM64 #913

merged 1 commit into from
Aug 4, 2023

Conversation

peterh
Copy link
Contributor

@peterh peterh commented Aug 4, 2023

Regression introduced in commit 3cd730b

@phlptp
Copy link
Collaborator

phlptp commented Aug 4, 2023

What was the error your were seeing?

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (792d892) 99.46% compared to head (0f3979b) 99.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #913   +/-   ##
=======================================
  Coverage   99.46%   99.46%           
=======================================
  Files          18       18           
  Lines        4108     4108           
=======================================
  Hits         4086     4086           
  Misses         22       22           
Files Changed Coverage Δ
include/CLI/impl/Argv_inl.hpp 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peterh
Copy link
Contributor Author

peterh commented Aug 4, 2023

The compile error is:

C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um\winnt.h(169): fatal error C1189: #error:  "No Target Architecture"

You can reproduce it when building for X86_64 by removing the _AMD64_ lines just above where I added the _ARM64_ lines.

@phlptp
Copy link
Collaborator

phlptp commented Aug 4, 2023

Ok, would having an ARM build on visual studio in a CI test have caught this error?

@peterh
Copy link
Contributor Author

peterh commented Aug 4, 2023

Ok, would having an ARM build on visual studio in a CI test have caught this error?

The question I think you were asking: Yes: Having a 64-bit ARM build on Visual Studio in a CI would have caught this error.

The question you actually asked: No: In Windows nomenclature, ARM without qualifications is a 32-bit ARM build, and this configuration was already supported. Only 64-bit ARM was broken. 😁

@phlptp
Copy link
Collaborator

phlptp commented Aug 4, 2023

That explains that then, I was pretty sure I had built this many times on Visual studio ARM but quite possibly not the ARM64 build.

@phlptp phlptp merged commit 9cd9890 into CLIUtils:main Aug 4, 2023
45 checks passed
@github-actions github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants