Do not depend on unbounded-delays on 64-bit architecture #344
Conversation
Bodigrim
force-pushed
the
no-unbounded-delays
branch
from
0ca53a5 to
b8ad86c
Compare
|
@VictorCMiraldo @andreasabel this is ready for review. |
There was a problem hiding this comment.
Pardon my delay, I just came back from holidays.
Shaving off a dependency is always nice and the changes are really small, thanks @Bodigrim!
| @@ -171,7 +177,9 @@ executeTest action statusVar timeoutOpt inits fins = mask $ \restore -> do | |||
| , resultTime = fromIntegral t | |||
| , resultDetailsPrinter = noResultDetails | |||
| } | |||
| fromMaybe timeoutResult <$> timeout t a | |||
| -- If compiled with unbounded-delays then t' :: Integer, otherwise t' :: Int | |||
| let t' = fromInteger (min (max 0 t) (toInteger (maxBound :: Int64))) | |||
There was a problem hiding this comment.
Is the max 0 t necessary? Both System.Timeout.timeout and its unbounded-delays counterpart are fmap Just when the specified delay is less than zero.
That being said, it doesn't hurt to keep it there. :)
There was a problem hiding this comment.
max 0 t is to prevent underflow. Imagine t = -(2^63+1) :: Integer: previously it meant no delay, but without max 0 t it would be cast to 2^63-1 :: Int, a huge timeout.
| ansi-terminal >= 0.9 | ||
|
|
||
| -- No reason to depend on unbounded-delays on 64-bit architecture | ||
| if(!arch(x86_64) && !arch(aarch64)) |
There was a problem hiding this comment.
This looks brittle.
What are the possible values of arch? I couldn't even find the list in the cabal docs...
At least, checking it this way (with a whitelist) is conservative. But new architectures might easily fall under the radar.
There was a problem hiding this comment.
Full list is here: https://hackage.haskell.org/package/Cabal-3.8.1.0/docs/src/Distribution.Simple.PreProcess.html#local-6989586621679281986
Potentially powerpc64, ia64 and s390x are also guaranteed to be 64-bit, but I decided to whitelist only architectures I have access to.
There was a problem hiding this comment.
I suggest to open an issue to potentially extend the list of recognized architectures, and whoever has access to one of them and can do testing can attack it.
64-bit RISC-V should fall into the same case in UnkindPartition#344 and should be added here. Tested locally.
64-bit RISC-V should fall into the same case in UnkindPartition#344 and should be added here. Tested locally.
64-bit RISC-V should fall into the same case in #344 and should be added here. Tested locally.
On 64-bit arch there is no point to depend on
unbounded-delays:(maxBound :: Int)microseconds are almost 300 000 years. Not a terribly big deal, but shaving off a dependency is always nice. Basically, CI jobs for all packages, which usetasty, have one less thing to compile.