You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "yuchanns (via GitHub)" <gi...@apache.org> on 2025/06/14 11:24:31 UTC
[PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
yuchanns opened a new pull request, #6293:
URL: https://github.com/apache/opendal/pull/6293
Refactor the Go bindings CI workflow to use a more streamlined approach:
- Replace complex matrix build configuration with simplified workflow
- Enhance Makefile with better organized targets and workspace management
- Update GitHub actions to use unified build process
- Simplify documentation by removing Windows-specific complex setup
Keep tests running on GitHub Actions and local development consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` does not affect files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
Additionally, `${{ github.workspace }}` defaults to `/home/runner/work/opendal/opendal`, not `/home/runner/_work`.
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
To clarify, if you check out with default arguments, it is cloned to `/home/runner/work/opendal/opendal`:
```yaml
steps:
- uses: actions/checkout@v4
```
If you check out with a specified path, for example `opendal`, then it is cloned to `/home/runner/work/opendal/opendal/opendal`:
```yaml
- uses: actions/checkout@v4
with:
path: opendal
```
In any case, a file always starts with `${{github.workspace}}` (`home/runner/work/opendal/opodal`) regardless of what working-directory is set by parent files.
In other words, the `working-directory` directive only works with `run` directive, and has noting to do with `uses` directive. In fact, if you combine `working-directory` and `uses` directly, it is invalid and cannot run.
In conclusion, it doesn't work:
```diff
jobs:
test:
# ...
+ defaults:
+ working-directory: opendal
steps:
- - uses: actions/checkout@v4
- uses: actions/checkout@v4
with:
path: opendal
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: "opendal-go-services"
- - uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- not work
+ - uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- work
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2154174435
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I don't think it's invalid at all. I'm very grateful for the time you spent discussing this with me in detail.
> Can we maintain consistency in CI?
It's challenging because different languages have varying practices in CI. I’m not sure how to improve it, but I would appreciate more input from contributors :)
P.S. I know little about other bindings, so maybe I should take some time to investigate and come up with better ideas.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153565682
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
The makefile requires a workspace layout:
```
opendal
openda-go-service
```
So ideally we don't need:
```yaml
- uses: actions/checkout@v4
```
We only need:
```yaml
- uses: actions/checkout@v4
with:
path: "opendal"
```
However, The docker setup requires an absolute path under `.`
https://github.com/apache/opendal/blob/69b61c52838f4661f6028b79e0a41c9e295c80de/.github/actions/test_behavior_binding_go/action.yaml#L40
If we do not clone the repository to `.`, it fails to setup the docker images.
P.S. This is not a problem of Docker, but a problem of setup file of OpenDAL services.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2155468069
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
OpenDAL bindings is customizable in these composite actions. It is exactly how you are building it.
I skimmed through the CI setup. Having two folder looks okay. I recommend setting jobs.<job_id>.working-directory: opendal which would save lots of diff. The same to the composite actions.
One more opportunity is to clean up the "cases" here:
.github/scripts/test_behavior/plan.py:77
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2156261975
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I have reduced some diff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2156261975
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I hav some diff reduced now.
BTW: I tried with:
```yaml
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: "../opendal-go-services
```
But GHA does not allow it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns merged PR #6293:
URL: https://github.com/apache/opendal/pull/6293
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2154147446
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
> it’s the responsibility of the Go binding maintainers to adjust the GHA for Go bindings in line with the main workflows.
Truly. I am grateful you are working on the Go binding and this initiative.
> To be honest, this has been our practice all along
I understand this is also true. I don't intend to block your PR but a discussion.
If you feel invalid, feel free to tell me. What I am getting is to take the whole repo into account, can we maintain some consistency in CI? This is useful for people who are less familiar about the CI to learn and understand setup faster. (Unfortunately, it's more design work).
I wouldn't think much from go binding's perspective since I know you also need C bindings. Welcome with your perspective.
-----
1. I get the two side-by-side folder setup. I assume it helps a lot for local development. I know Python binding is bringing an additional package into python binding itself. Maybe it's also easier for you? This leads to keeping the `opendal-go-services` along with the binding. I don't have insights about `opendal-go-services`. So I will rely on your judgment for which is better for developer experience.
2. In the composite actions, we can add additional `prefix` inputs with `defaults`. Maybe this will be useful for D/Zig bindings. I haven't validated this yet.
3. Of course, if none of these are useful, having a copy in CI about the code will work well too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157461589
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
The default `github.workspace` is usually `/home/runner/_work` where a file system mounts there.
The only way to load side by side is:
```
steps:
- uses: actions/checkout@v4
with:
path: opendal
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: opendal-go-services
```
When using actions, the path is relative to `${{ github.workspace }}`. The working directory remains unchanged. It also implies after setting `working-directory`, when using actions, any paths is still relative to `${{ github.workspace }}`.
So:
```
jobs:
test:
# ...
defaults:
working-directory: opendal
steps:
- uses: actions/checkout@v4
with:
path: opendal
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: "opendal-go-services"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153561479
##########
.github/actions/test_behavior_binding_go/action.yaml:
##########
@@ -37,13 +37,19 @@ runs:
using: composite
steps:
- name: Setup Test
- uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }}
+ uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }}
- name: Run Test Binding Go
shell: bash
- working-directory: bindings/go/tests/behavior_tests
run: |
- export DYLD_FALLBACK_LIBRARY_PATH=$DYLD_FALLBACK_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
- go test -v -run TestBehavior
+ if [ "${{ runner.os }}" == "macOS" ]; then
+ export DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
Review Comment:
Maybe I should remove this as the libffi in macOS is embedded now. FYI: https://github.com/jupiterrider/ffi?tab=readme-ov-file#macos
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2152939678
##########
bindings/go/Makefile:
##########
@@ -67,10 +61,13 @@ FINAL_LIB_FILE := $(GITHUB_WORKSPACE)/$(SERVICE_PKG)/libopendal_c.$(OS).$(ARCH).
# Build C library only when source files change
$(C_LIB_FILE): opendal/bindings/c/src/*.rs opendal/bindings/c/Cargo.toml
Review Comment:
Just curious, do Go projects use Makefile mostly? Or is this a choice initially?
##########
.github/actions/test_behavior_binding_go/action.yaml:
##########
@@ -37,13 +37,19 @@ runs:
using: composite
steps:
- name: Setup Test
- uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }}
+ uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }}
- name: Run Test Binding Go
shell: bash
- working-directory: bindings/go/tests/behavior_tests
run: |
- export DYLD_FALLBACK_LIBRARY_PATH=$DYLD_FALLBACK_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
- go test -v -run TestBehavior
+ if [ "${{ runner.os }}" == "macOS" ]; then
+ export DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
Review Comment:
README will be outdated for macOS. I am not familiar with Go's FFI. Will ffi library scan for some common places?
##########
.github/actions/test_behavior_binding_go/action.yaml:
##########
@@ -37,13 +37,19 @@ runs:
using: composite
steps:
- name: Setup Test
- uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }}
+ uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }}
- name: Run Test Binding Go
shell: bash
- working-directory: bindings/go/tests/behavior_tests
run: |
- export DYLD_FALLBACK_LIBRARY_PATH=$DYLD_FALLBACK_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
- go test -v -run TestBehavior
+ if [ "${{ runner.os }}" == "macOS" ]; then
+ export DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
+ ln -s /opt/homebrew/opt/libffi/lib/libffi.8.dylib \$(PWD)/opendal/bindings/go/tests/behavior_tests/
+ fi
+ if [ "${{ runner.os }}" == "Windows" ]; then
+ rm /usr/bin/link.exe
Review Comment:
Maybe add a comment?
##########
bindings/go/Makefile:
##########
@@ -84,8 +81,23 @@ $(FINAL_LIB_FILE): $(C_LIB_FILE)
@echo "Cleaning up intermediate directory..."
@rm -rf $(GITHUB_WORKSPACE)/libopendal_c_$(VERSION)_$(SERVICE)_$(OS)
-# Generate services target now just depends on the final file
-generate-services: $(FINAL_LIB_FILE)
+# Setup Go workspace for the service
+setup-workspace-$(SERVICE):
+ @echo "Setting up Go workspace for service: $(SERVICE)"
+ go work use $(GITHUB_WORKSPACE)/$(SERVICE)
+ @echo "Setting up Go module replacement for service: $(SERVICE)"
+ cd opendal/bindings/go/tests/behavior_tests && \
+ go mod edit -replace=github.com/apache/opendal-go-services/$(SERVICE)=$(GITHUB_WORKSPACE)/$(SERVICE) && \
+ go mod tidy
+
+# Generate scheme test file
+generate-scheme-$(SERVICE):
+ @echo "Generating scheme test file..."
+ @mkdir -p opendal/bindings/go/tests/behavior_tests
+ @printf '/*\n * Licensed to the Apache Software Foundation (ASF) under one\n * or more contributor license agreements. See the NOTICE file\n * distributed with this work for additional information\n * regarding copyright ownership. The ASF licenses this file\n * to you under the Apache License, Version 2.0 (the\n * "License"); you may not use this file except in compliance\n * with the License. You may obtain a copy of the License at\n *\n * http://www.apache.org/licenses/LICENSE-2.0\n *\n * Unless required by applicable law or agreed to in writing,\n * software distributed under the License is distributed on an\n * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n * KIND, either express or implied. See the License for the\n * specific language governing permissions and limitations\n * under the License.\n */\n// generated by github.com/apache/opendal/bindings/go\n\npackage opendal_test\n\nimport (\n\topendal "github.com/apache/opendal/bindings/go"\n\t"github.com/apach
e/opendal-go-services/$(SERVICE)"\n)\n\n// Add more schemes for behavior tests here.\nvar schemes = []opendal.Scheme{\n\t$(SERVICE).Scheme,\n}\n' > opendal/bindings/go/tests/behavior_tests/scheme_test.go
Review Comment:
If we can't have a heredoc, maybe use `define`, `endef` and `export`. e.g.,
```
define GITIGNOREDS
*.o
depend
endef
SRCS = $(wildcard [a-z]*.c)
EXES = $(SRCS:.c=)
export GITIGNOREDS
.gitignore: $(SRCS)
echo $(EXES) | sed 's/ /\n/g' > $@
echo "$$GITIGNOREDS" >> $@
```
Read [reference](https://stackoverflow.com/a/7377522/1612428).
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
Why do we clone the repo to another folder?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2156261975
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I hav some diff reduced now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153566170
##########
bindings/go/Makefile:
##########
@@ -84,8 +81,23 @@ $(FINAL_LIB_FILE): $(C_LIB_FILE)
@echo "Cleaning up intermediate directory..."
@rm -rf $(GITHUB_WORKSPACE)/libopendal_c_$(VERSION)_$(SERVICE)_$(OS)
-# Generate services target now just depends on the final file
-generate-services: $(FINAL_LIB_FILE)
+# Setup Go workspace for the service
+setup-workspace-$(SERVICE):
+ @echo "Setting up Go workspace for service: $(SERVICE)"
+ go work use $(GITHUB_WORKSPACE)/$(SERVICE)
+ @echo "Setting up Go module replacement for service: $(SERVICE)"
+ cd opendal/bindings/go/tests/behavior_tests && \
+ go mod edit -replace=github.com/apache/opendal-go-services/$(SERVICE)=$(GITHUB_WORKSPACE)/$(SERVICE) && \
+ go mod tidy
+
+# Generate scheme test file
+generate-scheme-$(SERVICE):
+ @echo "Generating scheme test file..."
+ @mkdir -p opendal/bindings/go/tests/behavior_tests
+ @printf '/*\n * Licensed to the Apache Software Foundation (ASF) under one\n * or more contributor license agreements. See the NOTICE file\n * distributed with this work for additional information\n * regarding copyright ownership. The ASF licenses this file\n * to you under the Apache License, Version 2.0 (the\n * "License"); you may not use this file except in compliance\n * with the License. You may obtain a copy of the License at\n *\n * http://www.apache.org/licenses/LICENSE-2.0\n *\n * Unless required by applicable law or agreed to in writing,\n * software distributed under the License is distributed on an\n * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n * KIND, either express or implied. See the License for the\n * specific language governing permissions and limitations\n * under the License.\n */\n// generated by github.com/apache/opendal/bindings/go\n\npackage opendal_test\n\nimport (\n\topendal "github.com/apache/opendal/bindings/go"\n\t"github.com/apach
e/opendal-go-services/$(SERVICE)"\n)\n\n// Add more schemes for behavior tests here.\nvar schemes = []opendal.Scheme{\n\t$(SERVICE).Scheme,\n}\n' > opendal/bindings/go/tests/behavior_tests/scheme_test.go
Review Comment:
Thank you !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153997751
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I don't think it's a burden for other contributors. They don’t need to understand Go bindings or worry about `./opendal/.github/actions/setup`; it’s the responsibility of the Go binding maintainers to adjust the GHA for Go bindings in line with the main workflows.
To be honest, this has been our practice all along. If someone encounters issues related to Go bindings in an unrelated PR, they collaborate with the Go binding maintainers (currently, that’s me). For your reference:
- https://github.com/apache/opendal/pull/5171
- https://github.com/apache/opendal/pull/6128
As seen in this PR, I did not request a change in behavior for `./.github/services/${{ inputs.service }}/${{ inputs.setup }}`; instead, I took the initiative to adapt it by cloning it into another folder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2154174435
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I don't think it's invalid at all. I'm very grateful for the time you spent discussing this with me in detail.
> Can we maintain some consistency in CI?
It's challenging since languages have different practices in CI. I’m not sure how to improve it, but I would appreciate more input from contributors :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2158279159
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
Yes, uses directive is the same as other with arguments.
It's a big letdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2155468069
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
OpenDAL bindings is customizable in these composite actions. It is exactly how you are building it.
I skimmed through the CI setup. Having two folder looks okay. I recommend setting jobs.<job_id>.working-directory: opendal which would save lots of diff. The same to the composite actions.
One more opportunity is to clean up the "cases" here:
.github/scripts/test_behavior/plan.py:77
What do you think about these?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153997751
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I don't thinks it's a burden for other contributors. They don’t need to understand Go bindings or worry about `./opendal/.github/actions/setup`; it’s the responsibility of the Go binding maintainers to adjust the GHA for Go bindings in line with the main workflows.
TBH, this has been our practice all along. If someone encounters issues related to Go bindings in an unrelated PR, they collaborate with the Go binding maintainers (currently, that’s me). FYI:
- https://github.com/apache/opendal/pull/5171
- https://github.com/apache/opendal/pull/6128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` has no effect on files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` does not affect files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
Additionally, `${{ github.workspace }}` defaults to `/home/runner/work/opendal/opendal`, not `/home/runner/_work`.
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
To clarify, if you check out with default arguments, it is cloned to `/home/runner/work/opendal/opendal`:
```yaml
steps:
- uses: actions/checkout@v4
```
If you check out with a specified path, for example `opendal`, then it is cloned to `/home/runner/work/opendal/opendal/opendal`:
```yaml
- uses: actions/checkout@v4
with:
path: opendal
```
In any case, a file always starts with `${{github.workspace}}` (`home/runner/work/opendal/opodal`) regardless of what working-directory is set by parent files.
In other words, the `working-directory` directive only works with `run` directive, and has noting to do with `uses` directive. In fact, if you combine `working-directory` and `uses` directly, it is invalid and cannot run.
In conclusion, it doesn't work:
```diff
jobs:
test:
# ...
+ defaults:
+ working-directory: opendal
steps:
- - uses: actions/checkout@v4
- uses: actions/checkout@v4
with:
path: opendal
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: "opendal-go-services"
- - uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- not work
+ - uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- work
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on PR #6293:
URL: https://github.com/apache/opendal/pull/6293#issuecomment-2990302414
Ouch! We hit API rate limits
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2158279159
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
Yes, uses directive is the same as other with arguments.
It's a big letdown.
We can move on with what you build. The PR is looking great.
For this particular invocation of checkout, can we do "ln -s . opendal"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2155468069
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
OpenDAL bindings CI is customizable in actions. This is exactly how you are building it.
One more opportunity is to clean up the entry path:
.github/scripts/test_behavior/plan.py:77
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` does not affect files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
Additionally, `${{ github.workspace }}` defaults to `/home/runner/work/opendal/opendal`, not `/home/runner/_work`.
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
To clarify, if you check out with default arguments, it is cloned to `/home/runner/work/opendal/opendal`:
```yaml
steps:
- uses: actions/checkout@v4
```
If you check out with a specified path, for example `opendal`, then it is cloned to `/home/runner/work/opendal/opendal/opendal`:
```yaml
- uses: actions/checkout@v4
with:
path: opendal
```
In any case, a file always starts with `${{github.workspace}}` (`home/runner/work/opendal/opodal`) regardless of what working-directory is set by parent files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153581492
##########
.github/actions/test_behavior_binding_go/action.yaml:
##########
@@ -37,13 +37,19 @@ runs:
using: composite
steps:
- name: Setup Test
- uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }}
+ uses: ./opendal/.github/services/${{ inputs.service }}/${{ inputs.setup }}
- name: Run Test Binding Go
shell: bash
- working-directory: bindings/go/tests/behavior_tests
run: |
- export DYLD_FALLBACK_LIBRARY_PATH=$DYLD_FALLBACK_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
- go test -v -run TestBehavior
+ if [ "${{ runner.os }}" == "macOS" ]; then
+ export DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/opt/homebrew/opt/libffi/lib
+ ln -s /opt/homebrew/opt/libffi/lib/libffi.8.dylib \$(PWD)/opendal/bindings/go/tests/behavior_tests/
+ fi
+ if [ "${{ runner.os }}" == "Windows" ]; then
+ rm /usr/bin/link.exe
Review Comment:
Make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153768990
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
> what are differences load repos side by side or use nested other repositories
From the perspective of GHA, there is no difference (This was excatly what we do before this PR). However, in local development, nested repositories can cause issues when committing changes, as you need to be careful not to commit unwanted repositories.
I want to maintain a consistent workspace layout between GHA and local development so that they share the same structure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153559254
##########
bindings/go/Makefile:
##########
@@ -67,10 +61,13 @@ FINAL_LIB_FILE := $(GITHUB_WORKSPACE)/$(SERVICE_PKG)/libopendal_c.$(OS).$(ARCH).
# Build C library only when source files change
$(C_LIB_FILE): opendal/bindings/c/src/*.rs opendal/bindings/c/Cargo.toml
Review Comment:
Not usually. In a CGO case, the developer needs to create C bindings, generate a Go artifact, and set up the workspace. Having a Makefile can simplify this process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153721657
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I see. Personally I would go with a variable in makefile. In the Action runtime, github.workspace (probably GITHUB_WORKSPACE) is the action/checkout's default path.
I don't think either your PR or my idea is the best solution in terms of maintenance - GitHub Actions is highly flexible and takes time to debug. I think we should get inputs on how we want to pull other code repos first.
This repo is going with a direction towards monorepo. The complexity will only grow. Some ideas in my mind:
- we append a directory prefix in composite actions
- enforce side-loading other repositories
- this idea generates a secondary problem - what are differences load repos side by side or use nested other repositories
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153559254
##########
bindings/go/Makefile:
##########
@@ -67,10 +61,13 @@ FINAL_LIB_FILE := $(GITHUB_WORKSPACE)/$(SERVICE_PKG)/libopendal_c.$(OS).$(ARCH).
# Build C library only when source files change
$(C_LIB_FILE): opendal/bindings/c/src/*.rs opendal/bindings/c/Cargo.toml
Review Comment:
Not usually. In a CGO case, the developer needs to create C bindings, generate a Go artifact, and set up the workspace. Having a Makefile can simplify this process.
I choose Makefile to ensure consistency between GitHub Actions and Local development to avoid things like https://github.com/apache/opendal/issues/6279 happen again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153768990
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
> what are differences load repos side by side or use nested other repositories
From the perspective of GHA, there is no difference (Side-loading was excatly what we do before this PR). However, in local development, nested repositories can cause issues when committing changes, as you need to be careful not to commit unwanted repositories.
I want to maintain a consistent workspace layout between GHA and local development so that they share the same structure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153559254
##########
bindings/go/Makefile:
##########
@@ -67,10 +61,13 @@ FINAL_LIB_FILE := $(GITHUB_WORKSPACE)/$(SERVICE_PKG)/libopendal_c.$(OS).$(ARCH).
# Build C library only when source files change
$(C_LIB_FILE): opendal/bindings/c/src/*.rs opendal/bindings/c/Cargo.toml
Review Comment:
Not usually. In a CGO case, the developer needs to create C bindings, generate a Go artifact, and set up the workspace. Having a Makefile can simplify this process.
I choose Makefile to ensure consistency between GitHub Actions and Local development.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` does not affect files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
Additionally, `${{ github.workspace }}` defaults to `/home/runner/work/opendal/opendal`, not `/home/runner/_work`.
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
To clarify, if you check out with default arguments, it is cloned to `/home/runner/work/opendal/opendal`:
```yaml
steps:
- uses: actions/checkout@v4
```
If you check out with a specified path, for example `opendal`, then it is cloned to `/home/runner/work/opendal/opendal/opendal`:
```yaml
- uses: actions/checkout@v4
with:
path: opendal
```
In any case, a file always starts with `${{github.workspace}}` (`home/runner/work/opendal/opodal`) regardless of what working-directory is set by parent files.
In other words, the `working-directory` directive only works with `run` directive, and has noting to do with `uses` directive. In fact, if you combine `working-directory` and `uses` directly, it is invalid and cannot run.
In conclusion, it doesn't work:
```diff
jobs:
test:
# ...
+ defaults:
+ working-directory: opendal
steps:
- - uses: actions/checkout@v4
- uses: actions/checkout@v4
with:
path: opendal
- uses: actions/checkout@v4
with:
repository: "apache/opendal-go-services"
path: "opendal-go-services"
- - uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- not work
+ - uses: ./.github/services/${{ inputs.service }}/${{ inputs.setup }} # <-- work
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` has no effect on files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
BTW, `${{ github.workspace }}` is not defaults to `/home/runner/_work`, but `/home/runner/_work/opendal/opendal`
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` has no effect on files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
BTW, `${{ github.workspace }}` is not defaults to `/home/runner/_work`, but `/home/runner/work/opendal/opendal`
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2157926312
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
I'm afraid it doesn't work because the `defaults.run.working-directory` does not affect files included by the `use` directive.
```
Error: Can't find 'action.yml', 'action.yaml', or 'Dockerfile' in '/Users/runner/work/opendal/opendal/.github/actions/setup'. Did you forget to run actions/checkout before executing your local action?
```
FROM: https://github.com/apache/opendal/actions/runs/15769891273/job/44452823057?pr=6293
Additionally, `${{ github.workspace }}` defaults to `/home/runner/work/opendal/opendal`, not `/home/runner/_work`.
FYI: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-contents-of-the-github-context
> "workspace": "/home/runner/work/hello-world/hello-world",
To clarify, if you check out with default arguments, it is cloned to `/home/runner/work/opendal/opendal`:
```yaml
steps:
- uses: actions/checkout@v4
```
If you check out with a specified path, for example `opendal`, then it is cloned to `/home/runner/work/opendal/opendal/opendal`:
```yaml
- uses: actions/checkout@v4
with:
path: opendal
```
In any case, a file always starts with `${{github.workspace}}` (`home/runner/work/opendal/opodal`) regardless of what working-directory is set by parent files.
In other words, the `working-directory` directive only works with `run` directive, and has noting to do with `uses` directive. In fact, if you combine `working-directory` and `uses` directly, it is invalid and cannot run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153565682
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
The makefile requires a workspace layout:
```
opendal
openda-go-service
```
So ideally we don't need:
```yaml
- uses: actions/checkout@v4
```
We only need:
```yaml
- uses: actions/checkout@v4
with:
path: "opendal"
```
However, The docker setup requires an absolute path under `.`
https://github.com/apache/opendal/blob/69b61c52838f4661f6028b79e0a41c9e295c80de/.github/actions/test_behavior_binding_go/action.yaml#L40
If we do not clone the repository to `.`, it fails to setup the docker images.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on PR #6293:
URL: https://github.com/apache/opendal/pull/6293#issuecomment-2982582910
Hi, @erickguan Thank you for you kind review. Things almost done here. Please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "yuchanns (via GitHub)" <gi...@apache.org>.
yuchanns commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153559254
##########
bindings/go/Makefile:
##########
@@ -67,10 +61,13 @@ FINAL_LIB_FILE := $(GITHUB_WORKSPACE)/$(SERVICE_PKG)/libopendal_c.$(OS).$(ARCH).
# Build C library only when source files change
$(C_LIB_FILE): opendal/bindings/c/src/*.rs opendal/bindings/c/Cargo.toml
Review Comment:
Not usually. While in a CGO case, the developer needs to create C bindings, generate a Go artifact, and set up the workspace. Having a Makefile can simplify this process.
I choose Makefile to ensure consistency between GitHub Actions and Local development to avoid things like https://github.com/apache/opendal/issues/6279 happen again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ci(bindings/go): simplify and improve Go bindings test infrastructure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6293:
URL: https://github.com/apache/opendal/pull/6293#discussion_r2153894907
##########
.github/workflows/test_behavior_binding_go.yml:
##########
@@ -28,37 +28,24 @@ on:
type: string
jobs:
- set-build:
- runs-on: ubuntu-latest
- outputs:
- build: ${{ steps.set-matrix-build.outputs.build }}
- steps:
- - uses: actions/checkout@v4
- - id: set-matrix-build
- name: Setup Matrix Build
- run: |
- MATRIX=$(yq -o=json -I=0 '[.build[] | select(.os == "${{ inputs.os }}")]' .github/scripts/test_go_binding/matrix.yaml | sed 's/ //g')
- echo "Matrix:"
- echo "$MATRIX" | jq .
- echo "build=$MATRIX" >> $GITHUB_OUTPUT
-
test:
- needs: [set-build]
name: ${{ matrix.cases.service }} / ${{ matrix.cases.setup }}
runs-on: ${{ inputs.os }}
strategy:
fail-fast: false
matrix:
cases: ${{ fromJson(inputs.cases) }}
- build: ${{ fromJson(needs.set-build.outputs.build) }}
steps:
- uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ path: "opendal"
Review Comment:
> From the perspective of GHA, there is no difference (Side-loading was excatly what we do before this PR).
Ok. It has nuances for us.
Yes, I get what you want to do. What concerns me is that using internal GitHub Actions, `./opendal/.github/actions/setup` is different than `./.github/actions/setup`. This will be more work for other maintainers working on workflows, who are unaware of Go bindings. This isn't good. The problem is that we will take on more maintenance costs for the whole repo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org