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/20 20:30:47 UTC
[GitHub] [arrow] zeroshade opened a new pull request, #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
zeroshade opened a new pull request, #34647:
URL: https://github.com/apache/arrow/pull/34647
<!--
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
Protecting the Go arrow packages from failing on new architectures by ensuring the pure go implementation gets loaded for any architecture that isn't one of the explicit ones we have assembly for.
<!--
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.
-->
### What changes are included in this PR?
changing the go build constraint
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
--
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 pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1477088463
there are unit tests you can run via `go test`. You could also try running `ci/scripts/go_test.sh` if you like.
--
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 pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1476891419
@clausecker Please try building this branch. I've only tested using go1.20 and `GOOS=freebsd GOARCH=riscv64 go build` as I don't have access to a riscv64 system. But with that I was able to reproduce the failing build you showed in the issue, and then use this fix to make it build successfully.
--
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] clausecker commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "clausecker (via GitHub)" <gi...@apache.org>.
clausecker commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1477998135
I am building with go 1.20, which is the most recent version. I haven't checked the source in detail. It is possible to use both types of build constraints at the same time and `go fix` (I think) should fix the code to have both in there if I recall correctly.
--
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 #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1480206103
Benchmark runs are scheduled for baseline = 532b9a57cab4fa1f88438fd0bd79cb7eb8aa2df3 and contender = ce0d20c06aeb5c484888422cb4ce42e4d24c588c. ce0d20c06aeb5c484888422cb4ce42e4d24c588c 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/ea479eb951294945bdf1e4dcb76b7c64...49cb10bb626d4f5092bfeadf8843ad7b/)
[Finished :arrow_down:0.39% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/1c52b10d13534b1dac813100ce75c7de...89571499569947c4a98cfeca5340c4c2/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/92f9cd717f7a4a6e98d24261bcbcfba5...5fa60d3d559e4aa8a1d964874520b3d4/)
[Finished :arrow_down:0.35% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/c2818ce1293342f5984958ec959cdc82...292de9cc282c43b6abe1ed4b0411c2eb/)
Buildkite builds:
[Finished] [`ce0d20c0` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2555)
[Finished] [`ce0d20c0` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2585)
[Finished] [`ce0d20c0` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2553)
[Finished] [`ce0d20c0` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2576)
[Finished] [`532b9a57` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2554)
[Finished] [`532b9a57` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2584)
[Finished] [`532b9a57` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2552)
[Finished] [`532b9a57` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2575)
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
[GitHub] [arrow] clausecker commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "clausecker (via GitHub)" <gi...@apache.org>.
clausecker commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1476961467
Thanks, will check. Is there are test suite or something I can 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1479842541
@clausecker I ran `go fix` on the whole directory there, can you try again and confirm you don't get that `invalid build constraint` error anymore?
--
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 #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #34647:
URL: https://github.com/apache/arrow/pull/34647
--
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 pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1477957456
Thankfully `modernc.org/sqlite` is only used for the flight sql example package, so likely shouldn't affect most consumers.
For the issue with `non-alphanumeric build constraints` what version of Go were you using there? Since we intend for building against older versions of Go too, i'm guessing I need to modify the `+build` constraint since the syntax using `||` and `&&` in the build constraints was added in go 1.16 i think....
--
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] github-actions[bot] commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1476890219
* Closes: #34629
--
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] clausecker commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "clausecker (via GitHub)" <gi...@apache.org>.
clausecker commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1477832280
Package `internal` now builds. Others do not, largely because you depend on `modernc.org/sqlite` which is not available for this architecture. A `go test ./internal/...` yielded:
```
# github.com/apache/arrow/go/v12/internal/utils
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: ||
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: (!amd64
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: &&
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: &&
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: &&
internal/utils/transpose_ints_noasm.go:20:1: invalid non-alphanumeric build constraint: !ppc64le)
```
which is something you might want to look into, too.
Thank you for the 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] clausecker commented on pull request #34647: GH-34629: [Go] Fix transpose_ints to work on riscv64-freebsd
Posted by "clausecker (via GitHub)" <gi...@apache.org>.
clausecker commented on PR #34647:
URL: https://github.com/apache/arrow/pull/34647#issuecomment-1479924816
I can confirm.
--
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