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

(WIP) ci(bindings/go): add windows to matrix #5066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hezhangjian
Copy link
Member

Main Issue #4892

@hezhangjian hezhangjian changed the title ci(bindings/go): add windows to matrix [WIP] ci(bindings/go): add windows to matrix Aug 28, 2024
@yuchanns yuchanns mentioned this pull request Oct 14, 2024
24 tasks
@Xuanwo
Copy link
Member

Xuanwo commented Jan 13, 2025

Hi, @yuchanns, would you like to take a review?

@hezhangjian hezhangjian changed the title [WIP] ci(bindings/go): add windows to matrix ci(bindings/go): add windows to matrix Jan 13, 2025
@yuchanns
Copy link
Member

yuchanns commented Jan 13, 2025

It failed at Setup Target.

3s
Run rustup target add $TARGET
error: error: the following required arguments were not provided:
  <target>...

IMO the pwsh doesn't support $TARGET, you can use $env:TARGET with a branch if: ${{ matrix.build.os == 'windows-latest' }}

@yuchanns
Copy link
Member

While it comes to the step Build C Binding, you will need to modify the dynamic suffix too:

      - name: Build C Binding
        working-directory: bindings/c
        env:
          VERSION: "latest"
          SERVICE: ${{ matrix.service }}
          TARGET: ${{ matrix.build.target }}
          CC: ${{ matrix.build.cc }}
          OS: ${{ matrix.build.os }}
        run: |
          cargo build --target $TARGET  --release
          DIR=$GITHUB_WORKSPACE/libopendal_c_${VERSION}_${SERVICE}_$TARGET
          mkdir $DIR
          if [ ${OS} == 'ubuntu-latest' ]; then
            SO=so
         # Add SO=dll
          else
            SO=dylib
          fi

@hezhangjian hezhangjian changed the title ci(bindings/go): add windows to matrix (WIP) ci(bindings/go): add windows to matrix Jan 13, 2025
@hezhangjian
Copy link
Member Author

@yuchanns Thanks, I will test it in my Windows Computer :)

@hezhangjian hezhangjian force-pushed the add-windows-test branch 5 times, most recently from 6a4858b to 08328f8 Compare January 13, 2025 07:43
@hezhangjian
Copy link
Member Author

hezhangjian commented Jan 13, 2025

@yuchanns Any idea about this? I think I am close.

It compiles opendal_c.dll, instead of libopendal_c.dll

   Compiling opendal v0.51.1 (D:\a\opendal\opendal\core)
    Finished `release` profile [optimized] target(s) in 2m 21s

    Directory: D:\a\opendal\opendal

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           1/13/2025  7:47 AM                libopendal_c_latest_fs_x86_64-pc-windows-msvc
zstd: can't stat ./target/x86_64-pc-windows-msvc/release/libopendal_c.dll : No such file or directory -- ignored 
image

@yuchanns
Copy link
Member

I think renaming it should just works.

@hezhangjian hezhangjian force-pushed the add-windows-test branch 2 times, most recently from f0ea5c1 to 5becbde Compare January 13, 2025 12:15
@hezhangjian
Copy link
Member Author

@yuchanns Sorry, can you give me some hint about this error?

Error: C:\Users\runneradmin\go\pkg\mod\github.com\apache\opendal-go-services\[email protected]\service.go:60:29: undefined: libopendalZst
# github.com/apache/opendal/bindings/go
Error: ..\..\ffi.go:32:28: undefined: purego.Dlopen
Error: ..\..\ffi.go:32:48: undefined: purego.RTLD_LAZY
Error: ..\..\ffi.go:32:65: undefined: purego.RTLD_GLOBAL
Error: ..\..\ffi.go:44:10: undefined: purego.Dlclose
Error: ..\..\ffi.go:86:21: undefined: purego.Dlsym
Error: ..\..\delete.go:58:25: undefined: unix.BytePtrFromString
Error: ..\..\operator.go:114:24: undefined: unix.BytePtrFromString
Error: ..\..\operator.go:1[80](https://github.com/apache/opendal/actions/runs/12747104741/job/35524528295?pr=5066#step:18:81):23: undefined: unix.BytePtrFromString
Error: ..\..\operator.go:184:25: undefined: unix.BytePtrFromString
Error: ..\..\operator_info.go:328:15: undefined: unix.BytePtrToString
Error: ..\..\operator_info.go:328:15: too many errors
FAIL	opendal_test [build failed]

@yuchanns
Copy link
Member

yuchanns commented Jan 13, 2025

Oops! That's an arch issue. Something needs to be solved in the go-services repo. There's no *_windows.go file now. I'll take a look tomorrow.

@hezhangjian
Copy link
Member Author

@yuchanns Could you please give me a chance to try? I would like to try to deep look opendal-go-services this week.

@yuchanns
Copy link
Member

Sure. It's yours:)

@yuchanns
Copy link
Member

Just for remind:

Error: ....\delete.go:58:25: undefined: unix.BytePtrFromString

The respective function in Windows is windows.UTF16PtrFromString. FYI: https://pkg.go.dev/golang.org/x/sys/windows

Maybe you can create a set of util_$OS.go files to implement common functions across multiple platforms.

util_windows.go
util_nix.go

And export PtrFromString.

@yuchanns
Copy link
Member

yuchanns commented Jan 15, 2025

Error: C:\Users\runneradmin\go\pkg\mod\github.com\apache\opendal-go-services\[email protected]\service.go:60:29: undefined: libopendalZst
# github.com/apache/opendal/bindings/go

Did you use Go Workspace to develop? Please follow the instructions of https://github.com/apache/opendal/tree/main/bindings/go#development.

Go Workspace is essential to development across opendal and opendal-go-services.

During the development of the Go binding, we do not rely on artifacts released by opendal-go-services. Instead, we build the latest artifacts from opendal-go-services within the Go Workspace. This is likely why you encountered the error below: there are currently no releases for Windows platform artifacts.

Error: ..\..\ffi.go:32:28: undefined: purego.Dlopen
Error: ..\..\ffi.go:32:48: undefined: purego.RTLD_LAZY
Error: ..\..\ffi.go:32:65: undefined: purego.RTLD_GLOBAL
Error: ..\..\ffi.go:44:10: undefined: purego.Dlclos

A positive example is that there are no releases for MacOS artifacts, yet the tests still work because Go Workspace newly generates the artifacts.

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

Successfully merging this pull request may close these issues.

3 participants