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.

Sign up for GitHub

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

Jump to bottom

Merge doxygen, comp libs, examples, and CMake from develop #5250

Closed
wants to merge 14 commits into from
Closed

Merge doxygen, comp libs, examples, and CMake from develop #5250

wants to merge 14 commits into from

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Jan 19, 2025

Doxygen upgraded
CMake - compression libs, compiler flags, examples, configuration

@byrnHDF byrnHDF added Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Component - Documentation Doxygen, markdown, etc. Component - Build CMake, Autotools Type - Improvement Improvements that don't add a new feature or functionality Type - Deprecation / Removal We strive for backward-compatibility, so it's worth noting this separately Merge - To 1.14.6 This should be considered for a 1.14.6 release labels Jan 19, 2025
@byrnHDF byrnHDF self-assigned this Jan 19, 2025
jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
doxygen/examples/H5I_examples.c Outdated
@@ -133,7 +133,7 @@ fail_dcpl:;
hid_t obj_id;

// register a new ID type
if ((type = H5Iregister_type(128, 1024, &free_func)) < 0) {
if ((type = H5Iregister_type2(1024, &free_func)) < 0) {
Copy link
Collaborator

@jhendersonHDF jhendersonHDF Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this change should be going into a minor release (the function was added for 2.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
CMakeLists.txt Outdated
@@ -147,8 +147,8 @@ if (HDF5_USE_FOLDERS)
endif ()
option (HDF5_NO_PACKAGES "CPACK - Disable packaging" OFF)
mark_as_advanced (HDF5_NO_PACKAGES)
option (ALLOW_UNSUPPORTED "Allow unsupported combinations of configure options" OFF)
mark_as_advanced (ALLOW_UNSUPPORTED)
option (HDF5_ALLOW_UNSUPPORTED "Allow unsupported combinations of configure options" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure whether changing the CMake option names is something that should go into a minor release. It made sense for a 2.0 release, but for a minor release it's a bit disruptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to change these because it fixed other issues in the build.

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
CMakeLists.txt Outdated
@@ -174,6 +175,7 @@ set (HDF5_JAVA_JNI_LIB_CORE "_java")

set (HDF5_LIB_CORENAME "${HDF5_LIB_BASE}")
set (HDF5_TEST_LIB_CORENAME "${HDF5_LIB_BASE}${HDF5_TEST_LIB_CORE}")
set (HDF5_TEST_PAR_LIB_CORENAME "${HDF5_LIB_BASE}${HDF5_TEST_PAR_LIB_CORE}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of a half-merge of develop since the changes for creating a testpar library aren't included in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
.github/workflows/cmake.yml
call-release-cmake-julia:
name: "CMake Julia Workflows"
uses: ./.github/workflows/julia-cmake.yml
# call-release-cmake-julia:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should decide whether we're going to leave the Julia tests disabled from develop or re-enable them and bring in the needed fix

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
.github/workflows/daily-schedule.yml Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want this scheduled workflow in a release branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
.github/workflows/hdfeos5.yml Outdated
workflow_dispatch:
push:
pull_request:
branches: [ hdf5_1_14 ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Branch name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
.github/workflows/linkchecker.yml Outdated
workflow_dispatch:
push:
pull_request:
branches: [ hdf5_1_14 ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Branch name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
CMakeLists.txt
endif ()

# For now, make sure we're using pthreads. Once Subfiling can be
# supported on Windows, we should allow Win32 threads as well
if (NOT ${Threads_FOUND} OR NOT ${CMAKE_USE_PTHREADS_INIT})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without the threading changes in develop, I'm not sure that changing the CMake here around the Subfiling VFD is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
doxygen/dox/VOLConnGuide.dox Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think most of the changes in this file were made specifically for 2.0 only

jhendersonHDF
jhendersonHDF reviewed Jan 20, 2025
View reviewed changes
src/H5Fmodule.h Outdated
* <a href="https://\DOXURL/_view_tools_command.html">Command-line Tools</a>.
* The HDF5 DDL grammar is described in the document \ref DDLBNF114.
* \ref ViewToolsCommand.
* The HDF5 DDL grammar is described in the document \ref DDLBNF200.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Back to 114

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@jhendersonHDF
Copy link
Collaborator

Still a lot more to review (about halfway through), but we might want to consider a different approach. By just merging certain parts of the codebase from develop, this seems to be in a bit of a half-merge state and with this many files it might be hard to review in a timely manner.

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Jan 20, 2025

Yeah looks like I missed a few things and got confused on some others. Will fix Tuesday morning.

@github-actions github-actions bot requested a review from epourmal as a code owner January 21, 2025 14:53
@byrnHDF byrnHDF closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhendersonHDF jhendersonHDF jhendersonHDF left review comments

@lrknox lrknox Awaiting requested review from lrknox lrknox is a code owner

@derobins derobins Awaiting requested review from derobins derobins is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@mattjala mattjala Awaiting requested review from mattjala mattjala is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

@epourmal epourmal Awaiting requested review from epourmal epourmal is a code owner

Assignees

@byrnHDF byrnHDF

Labels
Component - Build CMake, Autotools Component - Documentation Doxygen, markdown, etc. Merge - To 1.14.6 This should be considered for a 1.14.6 release Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Type - Deprecation / Removal We strive for backward-compatibility, so it's worth noting this separately Type - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@byrnHDF @jhendersonHDF