Add Windows support #930

sethrj · 2023-09-07T20:41:00Z

This adds non-CI-tested support for building and running on Windows. (Compiled with MS Visual Studio 2022, 14.37.32822.) All applications and tests build and pass in a near-minimal (only JSON and Googletest enabled) configuration.

Key compatibility issues are:

Fix many narrowing conversions, including one that would have introduced a serious bug due to integer overflow in the Dorman–Prince stepper constant
Use if constexpr rather than triad to fix compiler crash from template recursion. Before this, ipow relied on an optimization trick.
Fix some missing includes
Add conditional support for SIGUSR2 and isatty
Fix misuse of string_view::const_iterator as char const*
Work around some MSVC lambda capture scope issues
Normalize project source dir path written to celeritas test config file

Celeritas is about to become a dependency of SCALE to use ORANGE for CPU and GPU geometry. SCALE must compile on Windows.

Remove isatty for windows: ``` E:\scale\external\celeritas\src\corecel\io\ColorUtils.cc(13): fatal error C1083: Cannot open include file: 'unistd.h': No such file or directory ``` String view iterator is not a const char*: ``` E:\scale\external\celeritas\src\corecel\io\StringUtils.cc(66): note: 'std::basic_string_view<char,std::char_traits<char>>:: basic_string_view(const char *const ,const std::basic_string_view<char,std::char_traits<char>>::size_type) noexcept': canno t convert argument 1 from 'std::_String_view_iterator<_Traits>' to 'const char *const ' with [ _Traits=std::char_traits<char> ] ``` 'environ' is a macro on windows (boo) ``` E:\scale\external\celeritas\src\corecel\sys\Environment.cc(31): error C2267: '__p__environ': static functions with block scope are illegal ``` psapi must be included *after* windows.h ``` C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um\psapi.h(132): error C2146: syntax error: missing ';' before identifier 'WINAPI' ``` ptrdiff_t is implicitly converted to long ``` E:\scale\external\celeritas\src\corecel\sys\ScopedMem.cc(125): warning C4244: 'argument': conversion from 'ptrdiff_t' to 'long', possible loss of dat a ```

This might have relied on compiler optimization stopping the template instantiation early. With 'if constexpr' this should always be correct.

``` E:\scale\external\celeritas\test\corecel\math\NumericLimits.test.cc(40): error C2124: divide or mod by zero ```

``` E:\scale\external\celeritas\src\celeritas/field/DormandPrinceStepper.hh(144): warning C4146: unary minus operator applied to unsigned type, result st ill unsigned ```

``` E:\scale\external\celeritas\src\celeritas\geo\detail\BoundaryAction.hh(37): warning C4250: 'celeritas::detail::BoundaryAction': inherits 'celeritas::C oncreteAction::celeritas::ConcreteAction::action_id' via dominance E:\scale\external\celeritas\src\celeritas/global/ActionInterface.hh(151): note: see declaration of 'celeritas::ConcreteAction::action_id' E:\scale\external\celeritas\src\celeritas\geo\detail\BoundaryAction.hh(37): warning C4250: 'celeritas::detail::BoundaryAction': inherits 'celeritas::C oncreteAction::celeritas::ConcreteAction::label' via dominance E:\scale\external\celeritas\src\celeritas/global/ActionInterface.hh(154): note: see declaration of 'celeritas::ConcreteAction::label' E:\scale\external\celeritas\src\celeritas\geo\detail\BoundaryAction.hh(37): warning C4250: 'celeritas::detail::BoundaryAction': inherits 'celeritas::C oncreteAction::celeritas::ConcreteAction::description' via dominance ```

There are weird scoping rules about static const for lambdas. ``` E:\scale\external\celeritas\test\celeritas\track\TrackInit.test.cc(417): error C3493: 'num_tracks' cannot be implicitly captured because no default ca pture mode has been specified ```

``` E:\scale\external\celeritas\app\celer-sim\Transporter.cc(70): error C2065: 'SIGUSR2': undeclared identifier ```

``` E:\scale\external\celeritas\test\celeritas\em\UrbanMsc.test.cc(281): error C2397: conversion from 'const int' to 'T' requires a narrowing conversion with [T=celeritas::real_type ] ``` More conversion warnings

``` E:\scale\external\celeritas\test\corecel\cont\Range.test.cc(215): error: Value of: (std::is_same<std::underlying_type<pokemon::Pokemon>::type, unsigne d int>::value) Actual: false Expected: true ```

src/corecel/data/Collection.hh

pcanal · 2023-09-08T17:16:37Z

src/corecel/io/ColorUtils.cc

@@ -44,11 +46,13 @@ bool use_color()
            // Color is explicitly enabled
            return true;
        }
+#ifndef _WIN32


Is there a Windows equivalent to tell if there a terminal attached or not?

I'm not sure at all... even if there is, the Windows terminals don't natively support ANSI color sequences (only when running through something like git-bash). Either way, for the use cases of concern to SCALE, we won't be running through the command line.

humm ... isn't the code then

#ifdef _WIN32 return false; #else ... existing code... #endif

You could be running through git-bash but compiled against only regular windows. (This is how I was testing.) In that case the ANSI sequences are processed correctly because git-bash defines the TERM variable.

src/corecel/sys/Stream.cc

src/corecel/math/detail/MathImpl.hh

pcanal · 2023-09-12T09:16:15Z

src/corecel/sys/MemRegistry.hh

@@ -20,12 +20,12 @@ namespace celeritas
 //! SI prefix for multiples of 1024
 struct Kibi
 {
-    static CELER_CONSTEXPR_FUNCTION int value() { return 1024; }
+    static CELER_CONSTEXPR_FUNCTION long int value() { return 1024; }


What the purpose of that change? Rather than long int, do we mean int64_t ? (And almost more important how to we remember to use the right type in similar future construct?)

Int is 32-bit on windows and I wanted something "long"er. I try to steer away from special integer types for JSON I/O though now that I think about it, I'm not sure why. I'll unify all this.

I wanted something "long"er

Why does it matter here?

Because any memory change over 2GB overflows int on windows.

right, I should have look up the usage :).

But then why using size_t isn't the right type? (or make_signed<size_t>)?

it was the conversion between size_t and ptrdiff_t that was throwing me off. I've replaced it all with size_t.

src/corecel/sys/ScopedMem.cc

pcanal

LGTM. Thanks.

sethrj added 18 commits September 7, 2023 16:09

Fix truncation warning

f493f9b

Fix missing include

98364a4

Instantiate aligned plane explicitly

b2ca9fe

Use 'if constexpr' to avoid choking msvc

524564c

This might have relied on compiler optimization stopping the template instantiation early. With 'if constexpr' this should always be correct.

Define windows macros

a91783a

Fix compiler failure from divide by zero

ca78569

``` E:\scale\external\celeritas\test\corecel\math\NumericLimits.test.cc(40): error C2124: divide or mod by zero ```

Fix integer overflow in windows integer literal

c70a631

``` E:\scale\external\celeritas\src\celeritas/field/DormandPrinceStepper.hh(144): warning C4146: unary minus operator applied to unsigned type, result st ill unsigned ```

Fix conversion warnings

2ee19a1

Fix implicit cast in step conversion

6be5026

Fix MSVC implicit capture weirdness

ac55d87

There are weird scoping rules about static const for lambdas. ``` E:\scale\external\celeritas\test\celeritas\track\TrackInit.test.cc(417): error C3493: 'num_tracks' cannot be implicitly captured because no default ca pture mode has been specified ```

Fix use of SIGUSR2

ad5a197

``` E:\scale\external\celeritas\app\celer-sim\Transporter.cc(70): error C2065: 'SIGUSR2': undeclared identifier ```

Fix narrowing conversion

190c05d

``` E:\scale\external\celeritas\test\celeritas\em\UrbanMsc.test.cc(281): error C2397: conversion from 'const int' to 'T' requires a narrowing conversion with [T=celeritas::real_type ] ``` More conversion warnings

Fix missing symbol

80f8a1e

Work around windows environ definition

23096cf

Explicitly specify enum type

13d5367

``` E:\scale\external\celeritas\test\corecel\cont\Range.test.cc(215): error: Value of: (std::is_same<std::underlying_type<pokemon::Pokemon>::type, unsigne d int>::value) Actual: false Expected: true ```

Fix windows issues with paths

7b88218

sethrj added enhancement New feature or request core Software engineering infrastructure labels Sep 7, 2023

sethrj requested a review from pcanal September 7, 2023 20:41

sethrj added 2 commits September 7, 2023 17:22

Improve dependency logic for building inside another project

dc5f03c

fixup! Improve dependency logic for building inside another project

26b76d4

pcanal reviewed Sep 8, 2023

View reviewed changes

src/corecel/data/Collection.hh Show resolved Hide resolved

pcanal reviewed Sep 8, 2023

View reviewed changes

Clang-format

8c69634

sethrj requested a review from pcanal September 11, 2023 19:18

Work around dumb GCC attribute warning

0c5a57e

pcanal reviewed Sep 11, 2023

View reviewed changes

src/corecel/sys/Stream.cc Show resolved Hide resolved

Document

32359d2

sethrj requested a review from pcanal September 11, 2023 21:57

pcanal reviewed Sep 12, 2023

View reviewed changes

src/corecel/math/detail/MathImpl.hh Show resolved Hide resolved

pcanal reviewed Sep 12, 2023

View reviewed changes

src/corecel/sys/ScopedMem.cc Outdated Show resolved Hide resolved

sethrj added 4 commits September 12, 2023 07:03

Add default type to quantity based on underlying unit

e4eb459

Mark quantity functions as noexcept

95ee81e

Use size_t instead of ptrdiff_t and long int for scoped mem

adc543b

fixup! Add default type to quantity based on underlying unit

c583a9a

sethrj requested a review from pcanal September 12, 2023 14:40

Merge remote-tracking branch 'upstream/develop' into fix-windows-build

e117158

pcanal approved these changes Sep 12, 2023

View reviewed changes

iwyu

21ac428

sethrj force-pushed the fix-windows-build branch from 86849c9 to 21ac428 Compare September 13, 2023 01:40

sethrj merged commit 92133db into celeritas-project:develop Sep 13, 2023

sethrj deleted the fix-windows-build branch September 13, 2023 01:41

sethrj mentioned this pull request Sep 13, 2023

Hotfix: warning in ipow template instantiation #937

Merged

sethrj mentioned this pull request Oct 6, 2023

Hotfix: build errors due to if constexpr on NVCC < 11.5 #969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows support #930

Add Windows support #930

sethrj commented Sep 7, 2023 •

edited

Loading

pcanal Sep 8, 2023

sethrj Sep 8, 2023

pcanal Sep 12, 2023

sethrj Sep 12, 2023

pcanal Sep 12, 2023

sethrj Sep 12, 2023

pcanal Sep 12, 2023

sethrj Sep 12, 2023

pcanal Sep 12, 2023 •

edited

Loading

sethrj Sep 12, 2023

pcanal left a comment

Add Windows support #930

Add Windows support #930

Conversation

sethrj commented Sep 7, 2023 • edited Loading

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

pcanal Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcanal left a comment

Choose a reason for hiding this comment

sethrj commented Sep 7, 2023 •

edited

Loading

pcanal Sep 12, 2023 •

edited

Loading