You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/04/26 21:15:32 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #610: refactor(c): Simplify CI testing for cpp
WillAyd opened a new pull request, #610:
URL: https://github.com/apache/arrow-adbc/pull/610
The current test scripts look to be copy/paste from the build scripts. I think there's some unnecessary stuff there that can be removed, but figured it might be easier just to remove altogether to reduce layers
--
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-adbc] WillAyd commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189158148
##########
ci/scripts/cpp_test.ps1:
##########
@@ -18,16 +18,8 @@
$ErrorActionPreference = "Stop"
-$SourceDir = $Args[0]
-$BuildDir = $Args[1]
-$InstallDir = if ($Args[2] -ne $null) { $Args[2] } else { Join-Path $BuildDir "local/" }
-
-$BuildAll = $env:BUILD_ALL -ne "0"
-$BuildDriverManager = ($BuildAll -and (-not ($env:BUILD_DRIVER_MANAGER -eq "0"))) -or ($env:BUILD_DRIVER_MANAGER -eq "1")
-$BuildDriverFlightSql = ($BuildAll -and (-not ($env:BUILD_DRIVER_FLIGHTSQL -eq "0"))) -or ($env:BUILD_DRIVER_FLIGHTSQL -eq "1")
-$BuildDriverPostgreSQL = ($BuildAll -and (-not ($env:BUILD_DRIVER_POSTGRESQL -eq "0"))) -or ($env:BUILD_DRIVER_POSTGRESQL -eq "1")
-$BuildDriverSqlite = ($BuildAll -and (-not ($env:BUILD_DRIVER_SQLITE -eq "0"))) -or ($env:BUILD_DRIVER_SQLITE -eq "1")
-$BuildDriverSnowflake = ($BuildAll -and (-not ($env:BUILD_DRIVER_SNOWFLAKE -eq "0"))) -or ($env:BUILD_DRIVER_SNOWFLAKE -eq "1")
Review Comment:
Ah I see. I missed that these were being used for ctest labels - just assumed we ran a blanket `ctest` and whatever was built prior to the test execution is what gets tested. Makes sense on second review
--
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-adbc] WillAyd commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1524399064
Yea if you look at the deleted test script it looks like it copy/pasted all of the build variables but doesn't a really use them.
If you want to keep the scripts happy to go that route - I think they will end up being minimal now that we can just execute ctest from the build directory
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1546170027
Actually, it does look like the Snowflake tests don't work?
```
[100%] Built target adbc-driver-snowflake-test
Install the project...
-- Install configuration: "Debug"
-- Installing: /home/runner/work/arrow-adbc/arrow-adbc/build/local/lib/libadbc_driver_snowflake.so.4.0.0
-- Installing: /home/runner/work/arrow-adbc/arrow-adbc/build/local/lib/libadbc_driver_snowflake.so
-- Up-to-date: /home/runner/work/arrow-adbc/arrow-adbc/build/local/lib/libadbc_driver_snowflake.so
-- Installing: /home/runner/work/arrow-adbc/arrow-adbc/build/local/lib/libadbc_driver_snowflake.so.4
-- Installing: /home/runner/work/arrow-adbc/arrow-adbc/build/local/lib/pkgconfig/adbc-driver-snowflake.pc
~/work/arrow-adbc/arrow-adbc
~/work/arrow-adbc/arrow-adbc ~/work/arrow-adbc/arrow-adbc
Test project /home/runner/work/arrow-adbc/arrow-adbc
No tests were found!!!
Errors while running CTest
```
--
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-adbc] lidavidm commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189020087
##########
ci/scripts/cpp_test.ps1:
##########
@@ -18,16 +18,8 @@
$ErrorActionPreference = "Stop"
-$SourceDir = $Args[0]
-$BuildDir = $Args[1]
-$InstallDir = if ($Args[2] -ne $null) { $Args[2] } else { Join-Path $BuildDir "local/" }
-
-$BuildAll = $env:BUILD_ALL -ne "0"
-$BuildDriverManager = ($BuildAll -and (-not ($env:BUILD_DRIVER_MANAGER -eq "0"))) -or ($env:BUILD_DRIVER_MANAGER -eq "1")
-$BuildDriverFlightSql = ($BuildAll -and (-not ($env:BUILD_DRIVER_FLIGHTSQL -eq "0"))) -or ($env:BUILD_DRIVER_FLIGHTSQL -eq "1")
-$BuildDriverPostgreSQL = ($BuildAll -and (-not ($env:BUILD_DRIVER_POSTGRESQL -eq "0"))) -or ($env:BUILD_DRIVER_POSTGRESQL -eq "1")
-$BuildDriverSqlite = ($BuildAll -and (-not ($env:BUILD_DRIVER_SQLITE -eq "0"))) -or ($env:BUILD_DRIVER_SQLITE -eq "1")
-$BuildDriverSnowflake = ($BuildAll -and (-not ($env:BUILD_DRIVER_SNOWFLAKE -eq "0"))) -or ($env:BUILD_DRIVER_SNOWFLAKE -eq "1")
Review Comment:
Ah, we do actually want this though, because things like the verification scripts use this so we can build all the projects but only test some of them (since we can't reasonably expect people to have Snowflake credentials, for example)
--
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-adbc] lidavidm merged pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1526839126
Thanks!
I'm not sure what's going on with Windows here...I can get a Windows VM to take a look but it won't be until mid next week (when I have access to an x64 machine 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] WillAyd commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1524430222
No problem - will go that route then
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1524405541
Hmm, I'd rather keep the scripts for consistency (and in case we do need to add more logic later down the line)
--
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-adbc] WillAyd commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189035518
##########
ci/scripts/cpp_test.ps1:
##########
@@ -18,16 +18,8 @@
$ErrorActionPreference = "Stop"
-$SourceDir = $Args[0]
-$BuildDir = $Args[1]
-$InstallDir = if ($Args[2] -ne $null) { $Args[2] } else { Join-Path $BuildDir "local/" }
-
-$BuildAll = $env:BUILD_ALL -ne "0"
-$BuildDriverManager = ($BuildAll -and (-not ($env:BUILD_DRIVER_MANAGER -eq "0"))) -or ($env:BUILD_DRIVER_MANAGER -eq "1")
-$BuildDriverFlightSql = ($BuildAll -and (-not ($env:BUILD_DRIVER_FLIGHTSQL -eq "0"))) -or ($env:BUILD_DRIVER_FLIGHTSQL -eq "1")
-$BuildDriverPostgreSQL = ($BuildAll -and (-not ($env:BUILD_DRIVER_POSTGRESQL -eq "0"))) -or ($env:BUILD_DRIVER_POSTGRESQL -eq "1")
-$BuildDriverSqlite = ($BuildAll -and (-not ($env:BUILD_DRIVER_SQLITE -eq "0"))) -or ($env:BUILD_DRIVER_SQLITE -eq "1")
-$BuildDriverSnowflake = ($BuildAll -and (-not ($env:BUILD_DRIVER_SNOWFLAKE -eq "0"))) -or ($env:BUILD_DRIVER_SNOWFLAKE -eq "1")
Review Comment:
Will take another look. Shouldn't these be in the build script not the test script if anything though?
--
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-adbc] lidavidm commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189901748
##########
ci/scripts/cpp_test.ps1:
##########
@@ -18,16 +18,8 @@
$ErrorActionPreference = "Stop"
-$SourceDir = $Args[0]
-$BuildDir = $Args[1]
-$InstallDir = if ($Args[2] -ne $null) { $Args[2] } else { Join-Path $BuildDir "local/" }
-
-$BuildAll = $env:BUILD_ALL -ne "0"
-$BuildDriverManager = ($BuildAll -and (-not ($env:BUILD_DRIVER_MANAGER -eq "0"))) -or ($env:BUILD_DRIVER_MANAGER -eq "1")
-$BuildDriverFlightSql = ($BuildAll -and (-not ($env:BUILD_DRIVER_FLIGHTSQL -eq "0"))) -or ($env:BUILD_DRIVER_FLIGHTSQL -eq "1")
-$BuildDriverPostgreSQL = ($BuildAll -and (-not ($env:BUILD_DRIVER_POSTGRESQL -eq "0"))) -or ($env:BUILD_DRIVER_POSTGRESQL -eq "1")
-$BuildDriverSqlite = ($BuildAll -and (-not ($env:BUILD_DRIVER_SQLITE -eq "0"))) -or ($env:BUILD_DRIVER_SQLITE -eq "1")
-$BuildDriverSnowflake = ($BuildAll -and (-not ($env:BUILD_DRIVER_SNOWFLAKE -eq "0"))) -or ($env:BUILD_DRIVER_SNOWFLAKE -eq "1")
Review Comment:
That was only added recently after I realized we depend on that in verification :grimacing:
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1524406022
But if they have a bunch of unnecessary logic we can trim them
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1524373410
The test scripts run ctest, the build scripts run cmake - these are different unless you're seeing something different?
--
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-adbc] lidavidm commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189041194
##########
ci/scripts/cpp_test.ps1:
##########
@@ -18,16 +18,8 @@
$ErrorActionPreference = "Stop"
-$SourceDir = $Args[0]
-$BuildDir = $Args[1]
-$InstallDir = if ($Args[2] -ne $null) { $Args[2] } else { Join-Path $BuildDir "local/" }
-
-$BuildAll = $env:BUILD_ALL -ne "0"
-$BuildDriverManager = ($BuildAll -and (-not ($env:BUILD_DRIVER_MANAGER -eq "0"))) -or ($env:BUILD_DRIVER_MANAGER -eq "1")
-$BuildDriverFlightSql = ($BuildAll -and (-not ($env:BUILD_DRIVER_FLIGHTSQL -eq "0"))) -or ($env:BUILD_DRIVER_FLIGHTSQL -eq "1")
-$BuildDriverPostgreSQL = ($BuildAll -and (-not ($env:BUILD_DRIVER_POSTGRESQL -eq "0"))) -or ($env:BUILD_DRIVER_POSTGRESQL -eq "1")
-$BuildDriverSqlite = ($BuildAll -and (-not ($env:BUILD_DRIVER_SQLITE -eq "0"))) -or ($env:BUILD_DRIVER_SQLITE -eq "1")
-$BuildDriverSnowflake = ($BuildAll -and (-not ($env:BUILD_DRIVER_SNOWFLAKE -eq "0"))) -or ($env:BUILD_DRIVER_SNOWFLAKE -eq "1")
Review Comment:
We want to build all of the drivers, but only run some of the tests.
I suppose we could make it so that we don't build the tests in the first place, but for verification it'd be nice to at least validate that everything builds.
--
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-adbc] WillAyd commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1190080895
##########
dev/release/verify-release-candidate.sh:
##########
@@ -434,7 +434,8 @@ test_cpp() {
export BUILD_DRIVER_POSTGRESQL=0
# Snowflake driver requires snowflake creds for testing
export BUILD_DRIVER_SNOWFLAKE=0
- "${ADBC_DIR}/ci/scripts/cpp_test.sh" "${ADBC_SOURCE_DIR}" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
+ "${ADBC_DIR}/ci/scripts/cpp_build.sh" "${ADBC_SOURCE_DIR}" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
+ "${ADBC_DIR}/ci/scripts/cpp_test.sh" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
Review Comment:
This is a holdover from the previous design pattern where the environment variables were passed to just the build step, so you would build then `ctest` anything that was built. Should be removed since we are using the labels
--
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-adbc] lidavidm commented on pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#issuecomment-1540548415
FWIW, rebasing here should hopefully fix up CI
--
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-adbc] lidavidm commented on a diff in pull request #610: refactor(c): Simplify CI testing for cpp
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #610:
URL: https://github.com/apache/arrow-adbc/pull/610#discussion_r1189898142
##########
dev/release/verify-release-candidate.sh:
##########
@@ -434,7 +434,8 @@ test_cpp() {
export BUILD_DRIVER_POSTGRESQL=0
# Snowflake driver requires snowflake creds for testing
export BUILD_DRIVER_SNOWFLAKE=0
- "${ADBC_DIR}/ci/scripts/cpp_test.sh" "${ADBC_SOURCE_DIR}" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
+ "${ADBC_DIR}/ci/scripts/cpp_build.sh" "${ADBC_SOURCE_DIR}" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
+ "${ADBC_DIR}/ci/scripts/cpp_test.sh" "${ARROW_TMPDIR}/cpp-build" "${install_prefix}"
Review Comment:
Hmm, why are we building twice here?
--
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