-
-
Notifications
You must be signed in to change notification settings - Fork 307
Rework Fortran configure to allow cross compile overrides #5720
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
base: develop
Are you sure you want to change the base?
Conversation
The mingw cross-compile gets past the configure stage and then chokes on: because the values from the defaults are not correctly formatted. Once the syntax is correct then we can update the toolchain with the correct values. |
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.
📋 Pull Request Summary
Title: Rework Fortran configuration for cross-compilation supportFocus: Enhancing HDF5UseFortran.cmake to
handle cross-compilation scenarios by providing sensible default values for Fortran INTEGER and REAL kinds
when cross-compiling.
✅ Strengths
- Clear Problem Definition
- Addresses a specific pain point in cross-compilation workflows
- Targets mingw/gfortran cross-compilation which is commonly used
- Provides fallback defaults when runtime detection isn't possible
- Modular Implementation
- Clean conditional logic separating cross-compilation from native builds
- Maintains backward compatibility with existing build processes
- Uses CMake best practices for cross-compilation detection
- Verbose Configuration
- Good logging/messaging for configuration tracking
- Makes it easy to debug configuration issues
- Clear indication when defaults are being used
- Hardcoded Default Values
Potential issue: These defaults may not work for all compilers
set(H5_FORTRAN_DEFAULT_INTEGER_KINDS "1,2,4,8")
set(H5_FORTRAN_DEFAULT_REAL_KINDS "4,8")
Risk: Different Fortran compilers may have varying support for these kinds
Recommendation: Consider compiler-specific detection or documentation
- Limited Cross-Compiler Coverage
- Currently focuses on mingw/gfortran
- May need expansion for other cross-compilation scenarios (ARM, embedded systems, etc.)
- Default values might not be appropriate for all target architectures
- Testing Gaps
- Cross-compilation testing is inherently complex
- Need verification across multiple compiler/target combinations
- Risk of introducing regressions in native compilation
🔧 Technical Assessment
Code Quality: ⭐⭐⭐⭐☆
- Well-structured CMake code
- Clear separation of concerns
- Good error handling and messaging
Impact Analysis: ⭐⭐⭐⭐⭐
- Low Risk: Changes are isolated to Fortran configuration
- High Value: Enables important cross-compilation use cases
- Backward Compatible: Doesn't affect existing builds
Security: ⭐⭐⭐⭐⭐
- No security implications identified
- Actually reduces risk by providing sane defaults
📊 Recommendations
High Priority
- Expand Testing
Add CI jobs for multiple cross-compilation targets
- mingw-w64 (current)
- aarch64-linux-gnu
- arm-linux-gnueabihf
- Document Default Selection
Add comments explaining why these specific defaults were chosen
Reference: ISO Fortran standard, common compiler behavior, etc.
Medium Priority
- Compiler-Specific Defaults
Consider different defaults based on compiler
if(CMAKE_Fortran_COMPILER_ID MATCHES "GNU")
set(H5_FORTRAN_DEFAULT_INTEGER_KINDS "1,2,4,8")
elseif(CMAKE_Fortran_COMPILER_ID MATCHES "Intel")
# Intel-specific defaults
endif()
4. Runtime Validation
Add checks to verify the defaults actually work
Generate test program to validate kind availability
Low Priority
- Parameterization
Allow users to override defaults
set(HDF5_FORTRAN_CROSS_COMPILE_INTEGER_KINDS "${H5_FORTRAN_DEFAULT_INTEGER_KINDS}"
CACHE STRING "INTEGER kinds for cross-compilation")
🎯 Final Recommendation
APPROVE WITH MINOR REVISIONS ✅
Rationale:
- Addresses a legitimate need for cross-compilation support
- Implementation is clean and low-risk
- Benefits outweigh the concerns
- Issues can be addressed in follow-up PRs
Merge Conditions:
- ✅ Add comprehensive cross-compilation testing to CI
- ✅ Document the rationale for default kind selections
- ✅ Verify defaults work with target compilers
⚠️ Consider adding compiler-specific logic (future enhancement)
The requirement is to specify the correct values in a toolchain file. gfortran defaults are just in case. |
fortran/src/CMakeLists.txt
Outdated
if (NOT EXISTS "${HDF5_GENERATED_SOURCE_DIR}/H5fortran_types.F90") | ||
message (STATUS "No pre-generated H5fortran_types.F90 file found - generating one") | ||
# execute the H5match_types program | ||
if (CROSSCOMPILING) |
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.
How is CROSSCOMPILING different from CMAKE_CROSSCOMPILING?
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.
CMAKE_CROSSCOMPILING is the standard variable
Official CMake Documentation:
Source: CMAKE_CROSSCOMPILING variable documentation
Definition: "Variable that will be set to true by CMake if the CMAKE_SYSTEM_NAME variable has been manually set (i.e. in a toolchain file or as a cache entry from the cmake command line)."
Evidence from the PR: In config/toolchain/mingw64.cmake:2:
set (CMAKE_SYSTEM_NAME Windows)
This sets CMAKE_CROSSCOMPILING to TRUE automatically (since the host system is Linux/Ubuntu). The PR uses CROSSCOMPILING (without the CMAKE_ prefix), which is not a standard CMake variable and would need to be manually defined somewhere (which it isn't in this PR).
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.
CROSSCOMPILING
Looks like a typo.
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.
fixed.
Another possibility using defaults and toolchain file could overwrite.
See #5709 and #5718
The expectation is that cross-compiling will use a toolchain file to provide the correct values.
The defaults are only one way to provide defaults for a specific compiler.
NOTE: The supplied toolchain and Fortran pre-gen files are specific for mingw/gfortran - each platform/compiler will need to supply its own set of files/values.
Important
Rework
HDF5UseFortran.cmake
to support cross-compilation by providing default KINDs and sizes.HDF5UseFortran.cmake
to handle cross-compilation by providing default KINDs and sizes whenCMAKE_CROSSCOMPILING
is true.CMAKE_CROSSCOMPILING
to determine if default values should be used.{1,2,4,8,16}
and REAL KINDs{4,8,10,16}
.This description was created by
for 0b8f640. You can customize this summary. It will automatically update as commits are pushed.