-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add no_sign_request to s3 read test #778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
For #777. |
@@ -95,7 +95,7 @@ if (NOT HTSLIB_FOUND) | |||
ExternalProject_Add(ep_htslib | |||
PREFIX "externals" | |||
URL https://github.com/TileDB-Inc/m2w64-htslib-build/releases/download/1.20-0/m2w64-htslib-1.20-0.tar.gz | |||
URL_HASH SHA1=da39a3ee5e6b4b0d3255bfef95601890afd80709 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the mismatch happen? We should investigate it before changing the hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change shouldn't be needed for this PR though. That Windows CI has m2w64-htslib installed from conda. For some reason, in this PR, conda installs m2w64-htslib 1.20, which CMake can't find -- Could NOT find HTSlib (missing: HTSLIB_LIBRARIES HTSLIB_INCLUDE_DIR)
Whereas the last successful Windows CI build from last week installed m2w64-htslib 1.19 with conda, which CMake was able to find -- Found HTSlib: C:/Users/runneradmin/micromamba/envs/win-env/Library/lib/hts-3.lib
So 2 puzzles:
- Why is conda installing m2w64-1.20 this week but 1.19 just last week?
- Why can't CMake find htslib 1.20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a workaround if we can't quickly figure this out, we could pin m2w64-htslib to 1.19
Line 10 in 2271db0
- m2w64-htslib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing hash for m2w64-htslib-1.20-0.tar.gz
was wrong.
da39a3ee5e6b4b0d3255bfef95601890afd80709
is the hash from downloading the wrong URL.
$ wget -O- https://github.com/TileDB-Inc/m2w64-htslib-build/releases/download/foobar | sha1sum
--2024-09-27 15:51:28-- https://github.com/TileDB-Inc/m2w64-htslib-build/releases/download/foobar
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2024-09-27 15:51:29 ERROR 404: Not Found.
da39a3ee5e6b4b0d3255bfef95601890afd80709 -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought the hash mismatch issue should not block this PR, considering that it has happened before, and the artifact is in a first-party repository.
@teo-tsirpanis any idea why the windows build is failing after the htslib hash was fixed? |
Pinning Passinghttps://github.com/TileDB-Inc/TileDB-VCF/actions/runs/10905538566/job/30264621123
Failinghttps://github.com/TileDB-Inc/TileDB-VCF/actions/runs/11077183199/job/30781889277
|
@gspowley thanks for trying this. The whole situation is bizarre given that we didn't change anything that should have affected the Windows build. Note that even though it still failed, CMake was able to find htslib when 1.19 was installed, unlike when 1.20 was installed:
More evidence that the build environment has shifted without us realizing it. Today I synced my fork, and the latest commit that passed two weeks ago, 2271db0, failed on my fork, jdblischak@2271db0 |
@teo-tsirpanis do we still need this PR now that this was fixed upstream in TileDB-Inc/TileDB#5324? xref: #777 (comment) I still plan to continue troubleshoot the broken Windows build, but that is orthogonal to this update to the tests. |
With that Core PR the |
I'll close this PR. It looks like we'll hit a windows build issue in the future though. |
For future reference, the underlying cause of the failure here is that cmake can't find our externally-provided htslib. I don't see anything obvious in the cmake 3.30 release notes, but my first guess is that this is due to a change in the cmake version. From the builds in this comment:
([sc-46350] for follow-up time-permitting) |
Set
"vfs.s3.no_sign_request": True
in thetest_large_export_correctness
pytest, which reads public s3 data.