You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/03/07 16:44:54 UTC
[GitHub] [arrow] zeroshade opened a new pull request, #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
zeroshade opened a new pull request, #34488:
URL: https://github.com/apache/arrow/pull/34488
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Fix the failing CI runs
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128351119
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
Hmm, hardcoding the path like this seems brittle. Are we missing a `brew link` or something similar?
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
Review Comment:
nit: trailing whitespace
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128381603
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
IIRC we have something similar hardcoded in a bunch of places because there is no better solution?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128352620
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
+ ci/scripts/go_build.sh $(pwd)
- name: Test
shell: bash
- run: ci/scripts/go_test.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
+ ci/scripts/go_test.sh $(pwd)
Review Comment:
```suggestion
run: ci/scripts/go_test.sh $(pwd)
```
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
Review Comment:
nit:
```suggestion
cache-dependency-path: go/go.sum
```
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
This should update it for all following steps by setting it once :)
```suggestion
echo "PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128555081
##########
.github/workflows/go.yml:
##########
@@ -308,7 +308,9 @@ jobs:
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ echo "PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
Review Comment:
Ah, sorry about that. Hmm, the UI didn't seem to have shown that in the PR itself...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128388168
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
Yeah, that makes sense. Oh well...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128548518
##########
.github/workflows/go.yml:
##########
@@ -308,7 +308,9 @@ jobs:
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ echo "PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
Review Comment:
This doesn't affect in this step.
We need a separated step:
```yaml
- name: Prepare PKG_CONFIG_PATH
shell: bash
run: |
echo "PKG_CONFIG_PATH=$(brew --prefix openssl)/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
- name: Build
shell: bash
run: ci/scripts/go_build.sh $(pwd)
...
```
And this change causes YAML syntax error: https://github.com/apache/arrow/actions/runs/4358242702/workflow#L308
I think that the indentation of this line is a problem.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128371012
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
From what I found, it seems like it's explicitly an issue with the openssl brew package not placing `openssl.pc` in the same spot as other pkg-config paths. :( If there's a `brew link` or something that we can do, I haven't been able to find 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade merged pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #34488:
URL: https://github.com/apache/arrow/pull/34488
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128624483
##########
.github/workflows/go.yml:
##########
@@ -308,7 +308,9 @@ jobs:
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ echo "PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
Review Comment:
Also weird how the workflow didn't run on the last commit. Sorry for the indentation error that happens due to my comment suggestion 🙈
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34488:
URL: https://github.com/apache/arrow/pull/34488#discussion_r1128353287
##########
.github/workflows/go.yml:
##########
@@ -300,18 +300,22 @@ jobs:
with:
go-version: ${{ matrix.go }}
cache: true
- cache-dependency-path: go/go.sum
+ cache-dependency-path: go/go.sum
- name: Brew Install Arrow and pkg-config
shell: bash
run: brew install apache-arrow pkg-config
- name: Install staticcheck
run: go install honnef.co/go/tools/cmd/staticcheck@${{ matrix.staticcheck }}
- name: Build
shell: bash
- run: ci/scripts/go_build.sh $(pwd)
+ run: |
+ export PKG_CONFIG_PATH=/usr/local/Cellar/openssl@3/3.0.8/lib/pkgconfig:$PKG_CONFIG_PATH
Review Comment:
Ah, OpenSSL is special in that regard for Homebrew...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34488: MINOR: [Go][CI] Add openssl to macos CGO runs
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34488:
URL: https://github.com/apache/arrow/pull/34488#issuecomment-1468860486
Benchmark runs are scheduled for baseline = 6fdf1e520a761ee833f97241b3dd6687d23ef5ef and contender = 6a4c61f7163d9f10568132ecedf311acaa0a269e. 6a4c61f7163d9f10568132ecedf311acaa0a269e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/03bfe40d48064748b99bcba87b2851d6...b712fb6e91504f278957d9c653cb5975/)
[Failed :arrow_down:0.28% :arrow_up:0.06%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/6e031295fe6d4c73a747df5740a888e3...49fd658ef77b4332b3667e3529c0fe1c/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/07d4e47e303543509e6a1df22778a88f...5718a93e219141b49781aa8e442db001/)
[Finished :arrow_down:0.16% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/679fc20b962f449ab4ff8cd4734c11fe...f9549c3b8fb14c7dbe120fbfbe96523c/)
Buildkite builds:
[Finished] [`6a4c61f7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2490)
[Failed] [`6a4c61f7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2520)
[Finished] [`6a4c61f7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2488)
[Finished] [`6a4c61f7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2511)
[Finished] [`6fdf1e52` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2489)
[Failed] [`6fdf1e52` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2519)
[Finished] [`6fdf1e52` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2487)
[Finished] [`6fdf1e52` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2510)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org