Skip to content
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

Fix dataset creation failure on Ubuntu 24 #131

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

mattjala
Copy link
Collaborator

@mattjala mattjala commented Dec 13, 2024

  • Fix bad length value being provided to snprintf

A pointer value in RV_convert_dataspace_shape_to_JSON was cast to size_t in a way that didn't make sense, resulting in snprintf being provided an essentially random maximum length. For some reason, this only caused problems on Ubuntu 24.

  • Explicitly add Ubuntu 22.04 to CI

A recent update to the github runners changed ubuntu-latest from 22.04 to 24.04.

  • Merge autotools/cmake CI jobs into one using matrix to reduce duplication

  • Remove vol-tests submodule

As of hdf5#5170, the vol-tests don't successfully build anymore. These haven't been used for testing since the API tests were moved back into the library, so it makes sense to remove them.

  • Fix several type size warnings regarding hsize_t

@mattjala mattjala self-assigned this Dec 13, 2024
@mattjala mattjala force-pushed the hsds_log branch 3 times, most recently from 29c720b to d130105 Compare December 13, 2024 22:21
@mattjala mattjala changed the title Remove vol-tests submodule Make compatible with Ubuntu 24.04 Dec 13, 2024
@mattjala mattjala marked this pull request as draft December 13, 2024 22:24
@mattjala mattjala force-pushed the hsds_log branch 10 times, most recently from 862bc1c to cd9e087 Compare December 16, 2024 21:21
@mattjala mattjala changed the title Make compatible with Ubuntu 24.04 Fix dataset creation failure on Ubuntu 24 Dec 16, 2024
@mattjala mattjala marked this pull request as ready for review December 16, 2024 21:23
@mattjala mattjala added the bug Something isn't working label Dec 17, 2024
@@ -4125,8 +4124,7 @@ RV_curl_delete(CURL *curl_handle, server_info_t *server_info, const char *reques

strcpy(host_header, host_string);

curl_headers_local = curl_slist_append(
curl_headers_local, strncat(host_header, filename, host_header_len - strlen(host_string) - 1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these converted back to strcat calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lengths provided to strncat were pulled from the length of the target strings anyway, so having these be strncat didn't really accomplish anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, in this case yes they weren't doing much, though they're more future-proof than a regular strcat in case someone adds more things to the header in the future and forgets to allocate the space correctly. Really, I think most of these should just be switched to a single snprintf anyway to avoid most of the problems of having to manage the buffer size.

-DHDF5_BUILD_HL_LIB=ON \
-DBUILD_SHARED_LIBS=ON -DHDF5_ENABLE_SZIP_SUPPORT=OFF \
-DHDF5_TEST_API=ON \
-DHDF5_ENABLE_Z_LIB_SUPPORT=OFF \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, depending on which branch you're building this option could either be Z_LIB or ZLIB now. Though it shouldn't be a problem for develop as long as the default stays OFF

@jhendersonHDF jhendersonHDF merged commit 82f89c3 into HDFGroup:master Dec 18, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants