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: wasm module test #8099

Merged
merged 2 commits into from
Dec 16, 2024
Merged

fix: wasm module test #8099

merged 2 commits into from
Dec 16, 2024

Conversation

itaysk
Copy link
Contributor

@itaysk itaysk commented Dec 14, 2024

Description

I saw that the build in another PR failed, and wanted to investigate. I could reproduce it on my laptop also when pulling latest main. However no other workflow fails which I'm not sure why.

Error:


~/dev/trivy ❯ mage test:generatemodules

# github.com/aquasecurity/trivy/pkg/module/wasm
../../wasm/sdk.go:146:10: cannot use uintptr(size) (value of type uintptr) as int value in assignment

../../wasm/sdk.go:147:10: cannot use uintptr(size) (value of type uintptr) as int value in assignment

../../wasm/sdk.go:163:9: cannot use uintptr(size) (value of type uintptr) as int value in struct literal

../../wasm/sdk.go:164:9: cannot use uintptr(size) (value of type uintptr) as int value in struct literal

pkg/module/testdata/analyzer/analyzer.go:1: running "tinygo": exit status 1

Error: running "go generate pkg/module/testdata/analyzer/analyzer.go" failed with exit code 1

I'm not an expert in this :) , let me know if I got it right:
Offending code is trying to convert ptr/size pair from wasm world into a Go string.
tinygo docs says that Go's SliceHeader's Len and Cap are int and in tinygo they are uintptr (this is by design).
But the error tells us that when it tried to assign size, it got it as uintptr but it expected int (as if it is building with Go). Not sure how this happens becuase this code has a build constraint of tinygo.wasm.

Anyway, the same doc says we should no longer use SliceHeader (it's deprecated) and instead use unsafe.Slice. I noticed there is also now an unsafe.String which sounds even more fitting for this use case.
So I changed the offending code to use it and it now compiles (and also unit tests succeed).

As a side, this error occurs twice, but it seems like with a small refactor we can reuse the code, so I added a second commit that does that.

I'm still unsure of:

  1. why only this PR fails and other don't
  2. why my main is failing while others not (maybe tinygo version? I'm on tinygo version 0.34.0 darwin/arm64 (using go version go1.23.1 and LLVM version 18.1.2))
  3. why this code fails for a behavior that looks like was trying to compile using Go and not tinygo.

@knqyf263
Copy link
Collaborator

  1. why only this PR fails and other don't

It looks like your PR is failing due to the domain change from aquasecurity.github.io to trivy.dev, not TinyGo. I guess your local build failed for another reason (TinyGo), which confused you.

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -5,3 +5,3 @@
        	            	  DocumentName: (string) (len=11) "go-artifact",
        	            	- DocumentNamespace: (string) (len=95) "http://aquasecurity.github.io/trivy/filesystem/go-artifact-3ff14136-e09f-4df9-80ea-000000000005",
        	            	+ DocumentNamespace: (string) (len=76) "http://trivy.dev/filesystem/go-artifact-3ff14136-e09f-4df9-80ea-000000000005",
        	            	  ExternalDocumentReferences: ([]v2_3.ExternalDocumentRef) <nil>,
        	Test:       	TestMarshaler_Marshal/go_library_local
  1. why my main is failing while others not (maybe tinygo version? I'm on tinygo version 0.34.0 darwin/arm64 (using go version go1.23.1 and LLVM version 18.1.2))

Yes, TinyGo v0.32.0 seems to add a breaking change. I'm still using an older version.
#7568

Since our workflow uses TinyGo v0.31.1, I think your PR failed for another reason.

  1. why this code fails for a behavior that looks like was trying to compile using Go and not tinygo.

I'm not sure I get your point correctly, but it sounds like the other way around. This file has the constraint tinygo.wasm, which means it will be built with TinyGo + WASM only, not Go. So, it looks straightforward that the code fails due to TinyGo.

//go:build tinygo.wasm

@itaysk
Copy link
Contributor Author

itaysk commented Dec 15, 2024

thanks. I couldn't find the breaking change in tinygo, as their doc is still referring to the previous behavior, it seems. anyway, I thought that this PR will be a better solution for any of the behaviors (and will prevent people from making the same mistake), but I'll close it for now

@itaysk itaysk closed this Dec 15, 2024
@knqyf263
Copy link
Collaborator

knqyf263 commented Dec 15, 2024

Is there any reason you closed this PR? This change makes sense to me. We wanted to add this change, but nobody tried. We should merge this PR. Actually, I accepted the same change in a different project.

@itaysk itaysk reopened this Dec 16, 2024
@knqyf263 knqyf263 added this pull request to the merge queue Dec 16, 2024
Merged via the queue into aquasecurity:main with commit 2200f38 Dec 16, 2024
22 checks passed
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

Successfully merging this pull request may close these issues.

2 participants