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

TestGradient with "in_hsl_longer_hue" currently fails on ppc64le and s390x #3650

Closed
anthonyfok opened this issue Feb 15, 2024 · 6 comments
Closed

Comments

@anthonyfok
Copy link

anthonyfok commented Feb 15, 2024

esbuild v0.19.12 Debian package build currently fails on ppc64le and s390x due to TestGradient relating to in_hsl_longer_hue, as seen currently on https://buildd.debian.org/status/package.php?p=golang-github-evanw-esbuild:

The error messages are:

=== RUN   TestGradient/a_{_background:_linear-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     linear-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
=== RUN   TestGradient/a_{_background:_radial-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     radial-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
=== RUN   TestGradient/a_{_background:_conic-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     conic-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
=== RUN   TestGradient/a_{_background:_repeating-linear-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     repeating-linear-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
=== RUN   TestGradient/a_{_background:_repeating-radial-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     repeating-radial-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
=== RUN   TestGradient/a_{_background:_repeating-conic-gradient(in_hsl_longer_hue,_red,_green)_}_[lower]
    css_parser_test.go:870: 
        �[37m a {�[0m
        �[37m   background:�[0m
        �[37m     repeating-conic-gradient(�[0m
        �[37m       #ff0000,�[0m
        �[37m       #ef0078,�[0m
        �[37m       #df00df,�[0m
        �[37m       #6800cf,�[0m
        �[31m-      #0000c0,�[0m
        �[32m+      #0000bf,�[0m
        �[37m       #0058b0,�[0m
        �[37m       #00a0a0,�[0m
        �[37m       #009048,�[0m
        �[37m       #008000);�[0m
        �[37m }�[0m
        �[37m �[0m
--- FAIL: TestGradient (0.02s)
    --- PASS: TestGradient/a_{_background:_linear-gradient(red,_blue)_} (0.00s)
    ...
    --- FAIL: TestGradient/a_{_background:_linear-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    --- FAIL: TestGradient/a_{_background:_radial-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    --- FAIL: TestGradient/a_{_background:_conic-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    --- FAIL: TestGradient/a_{_background:_repeating-linear-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    --- FAIL: TestGradient/a_{_background:_repeating-radial-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    --- FAIL: TestGradient/a_{_background:_repeating-conic-gradient(in_hsl_longer_hue,_red,_green)_}_[lower] (0.00s)
    ...

comparing with the expected values in internal/css_parser/css_parser_test.go:

                expectPrintedLowerUnsupported(t, compat.GradientInterpolation,
                        "a { background: "+gradient+"(in hsl longer hue, red, green) }",
                        "a {\n  background:\n    "+gradient+"(\n      #ff0000,\n      #ef0078,\n      "+
                                "#df00df,\n      #6800cf,\n      #0000c0,\n      #0058b0,\n      "+
                                "#00a0a0,\n      #009048,\n      #008000);\n}\n", "")

it seems that we are getting an extraneous #0000bf right after instead of #0000c0 on s390x and ppc64el.

I haven't looked any deeper into it yet, but I suspect it has something to do with different "fused mulitply and add" (FMA) operations on different architectures, similar to the following issues:

I hope to find time to take a closer look on Debian's ppc64el and s390x porterboxes soon.

Thanks!

@anthonyfok
Copy link
Author

anthonyfok commented Feb 15, 2024

I just noticed that GitHub adds a nice colour preview to the hex colour values, so I decided to "play" here 😆:

Expected:

#ff0000
#ef0078
#df00df
#6800cf
#0000c0
#0058b0
#00a0a0
#009048
#008000

Got on ppc64el and s390x (slightly different results):

#ff0000
#ef0078
#df00df
#6800cf
#0000bf
#0058b0
#00a0a0
#009048
#008000

@anthonyfok anthonyfok changed the title TestGradient with "in_hsl_longer_hue" currently fails on ppc64el and s390x TestGradient with "in_hsl_longer_hue" currently fails on ppc64le and s390x Feb 16, 2024
@evanw
Copy link
Owner

evanw commented Feb 16, 2024

Thanks for the report. That's indeed not great. I can't just adjust the tests because esbuild aspires to always have deterministic output. Fixing this is going to be tricky and maybe a lot of work (not just adding the casts that https://go.dev/ref/spec#Floating_point_operators says to add, but more getting a test environment together and then verifying the fix).

@anthonyfok
Copy link
Author

Thanks for your reply!

Here is the workaround I am using to allow esbuild enters Debian testing: https://salsa.debian.org/go-team/packages/golang-github-evanw-esbuild/-/blob/debian/sid/debian/patches/0002-TestGradient-hsl-longer-hue-on-ppc64le-s390x.patch?ref_type=heads (which I am sure is not the solution you are looking for.)

About a test environment: A potential good news! I am able to reproduce the same discrepancy seen on ppc64le and s390x by running the following commands on my amd64 machine:

$ GOARCH=ppc64le go test ./internal/css_parser/...
$ GOARCH=s390x go test ./internal/css_parser/...

where QEMU (/usr/bin/qemu-ppc64le-static and /usr/bin/qemu-s390x-static, found in qemu-system-ppc and qemu-system-misc packages respectively on Debian) emulates the problem seemingly perfectly! Hope a simple sudo apt install qemu-user-static qemu-system-ppc qemu-system-misc or the equivalent for your favourite Linux distro would allow you to set up a test environment with minimal effort!

@anthonyfok
Copy link
Author

Good news! Preventing FMA in multiplyMatrices() alone allows the current "hsl longer" hue interpolation tests to pass (giving #0000c0 instead of #0000bf) on both ppc64le and s390x:

--- a/internal/css_parser/css_color_spaces.go
+++ b/internal/css_parser/css_color_spaces.go
@@ -361,9 +361,9 @@ func oklch_to_oklab(l float64, c float64, h float64) (float64, float64, float64)
 }
 
 func multiplyMatrices(A [9]float64, b0 float64, b1 float64, b2 float64) (float64, float64, float64) {
-	return A[0]*b0 + A[1]*b1 + A[2]*b2,
-		A[3]*b0 + A[4]*b1 + A[5]*b2,
-		A[6]*b0 + A[7]*b1 + A[8]*b2
+	return float64(A[0]*b0) + float64(A[1]*b1) + float64(A[2]*b2),
+		float64(A[3]*b0) + float64(A[4]*b1) + float64(A[5]*b2),
+		float64(A[6]*b0) + float64(A[7]*b1) + float64(A[8]*b2)
 }
 
 func delta_eok(L1 float64, a1 float64, b1 float64, L2 float64, a2 float64, b2 float64) float64 {

Tested on:

  • amd64 with GOARCH=ppc64le and GOARCH=s390x and QEMU emulation
  • real hardware (see https://db.debian.org/machines.cgi):
    • platti.debian.org, Debian’s ppc64el porterbox
    • zelenka.debian.org, Debian’s s390x porterbox

Of course, I don't guarantee 100% deterministic output for all possible cases out there, but at least it works for all existing test cases at the moment.

I am sorry I didn't make this into a pull request. Please feel free to commit the patch above directly if you see fit. :-)

Cheers!

@evanw
Copy link
Owner

evanw commented Feb 18, 2024

Thanks very much for the help with QEMU. I didn't realize it was that easy to integrate it with the Go compiler. I'm going to fix this in a more invasive way that does all color space math with a wrapper type around float64 to guarantee that the casts are used everywhere instead of just adding the minimal casts that make these failing test cases pass.

@evanw evanw closed this as completed in 5650831 Feb 18, 2024
@anthonyfok
Copy link
Author

You are amazing! Your commit 5650831 is a brilliant and complete solution! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants