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

Add Cygwin GitHub Action. #4112

Closed
wants to merge 7 commits into from
Closed

Add Cygwin GitHub Action. #4112

wants to merge 7 commits into from

Conversation

hyoklee
Copy link
Member

@hyoklee hyoklee commented Mar 11, 2024

CMake test passes if test/th5_system.c has the following patch:

#ifdef __CYGWIN__
#define _GNU_SOURCE
#endif

Autotools test fails with the following error:

Testing MDC/FILE related API calls                                    *FAILED*
check_file_mdc_api_calls: failure_mssg = "Unexpected internal config #3.".
Testing MDC API smoke check                                            -SKIP-
     Long tests disabled.
Testing MDC/FILE related API input errors                             *FAILED*
check_file_mdc_api_errs: failure_mssg = "H5Fcreate() failed.
".
Testing MDC/FAPL related API calls for paged aggregation strategy     *FAILED*
check_fapl_mdc_api_calls: failure_mssg = "H5Fcreate() failed.
".
Testing MDC/FILE related API calls for paged aggregation strategy     *FAILED*
check_file_mdc_api_calls: failure_mssg = "H5Fcreate() failed.
".
Testing MDC API smoke check for paged aggregation strategy             -SKIP-
     Long tests disabled.
Testing MDC/FILE related API input errors for paged aggregation strategy*FAILED*
check_file_mdc_api_errs: failure_mssg = "H5Fcreate() failed.

@byrnHDF
Copy link
Contributor

byrnHDF commented Mar 11, 2024

#define _GNU_SOURCE
That should be in ConfigueChecks.cmake and added to the global defines - not in code.

byrnHDF
byrnHDF reviewed Mar 11, 2024
View reviewed changes
Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

The yaml additions look good - Omly complaint is the name cwin-* maybe should be cygwin-*

@hyoklee hyoklee added the Component - Build CMake, Autotools label Mar 11, 2024
@byrnHDF
Copy link
Contributor

byrnHDF commented Mar 11, 2024

OH, yes tests may use different flags

byrnHDF
byrnHDF reviewed Mar 11, 2024
View reviewed changes
configure.ac Outdated
@@ -1519,6 +1519,10 @@ case "$host_cpu-$host_vendor-$host_os" in
AC_DEFINE([HAVE_MINGW], [1], [Define if using MinGW])
H5_CPPFLAGS="-D_GNU_SOURCE -D__USE_MINGW_ANSI_STDIO $H5_CPPFLAGS"
;;
*cygwin*)
AC_DEFINE([HAVE_CYGWIN], [1], [Define if using MinGW])
H5_CPPFLAGS="-D_GNU_SOURCE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need $H5_CPPFLAGS so that it is appeneded (see the linux section above - line 1514)

@byrnHDF byrnHDF marked this pull request as draft March 12, 2024 17:35
tbeu
tbeu reviewed Mar 12, 2024
View reviewed changes
.github/workflows/cygwin-cmake.yml
Comment on lines +41 to +42
cd build
ctest -T Build
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cd build
ctest -T Build
cmake --build build -- -j8

.github/workflows/cygwin-cmake.yml
Comment on lines +30 to +32
mkdir build
cd build
cmake -G Ninja \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mkdir build
cd build
cmake -G Ninja \
cmake -S . -B build -G Ninja \

.github/workflows/cygwin-cmake.yml
Comment on lines +48 to +49
cd build
ctest -T Test
Copy link
Contributor

@tbeu tbeu Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
cd build
ctest -T Test
ctest --test-dir build -T Test

Copy link
Member Author

Choose a reason for hiding this comment

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

@tbeu , thanks for taking the time to review my pull request! I also wanted to express my sincere gratitude for your work on maintaining the matio project. It's a valuable resource for the community. I'm happy to see that Cygwin is now a part of matio CI.

Copy link
Contributor

@tbeu tbeu Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, it's give and take. ❤️

(Cygwin via autotools was already tested in matio via Appveyor.)

@byrnHDF byrnHDF mentioned this pull request Mar 13, 2024
Merged
@byrnHDF
Copy link
Contributor

byrnHDF commented Mar 14, 2024

#4131 supersedes this PR - closing

@byrnHDF byrnHDF closed this Mar 14, 2024
@hyoklee hyoklee deleted the hyoklee-hdf5-157 branch March 15, 2024 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbeu tbeu tbeu left review comments

@byrnHDF byrnHDF byrnHDF approved these changes

@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

@jhendersonHDF jhendersonHDF Awaiting requested review from jhendersonHDF jhendersonHDF 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

Assignees

@byrnHDF byrnHDF

Labels
Component - Build CMake, Autotools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hyoklee @byrnHDF @tbeu @derobins