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

Merge v3.2.4 release candidate for final release #966

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Nov 22, 2024

This release candidate has been available for 7 weeks and there are no additional changes requested.

Requires merge to support finalization of the draft release.

@@ -49,8 +49,7 @@ jobs:
shell: bash -l {0}
run: |
conda install curl eigen
conda install -c conda-forge hdf5=1.10.6
conda remove -y yaml-cpp

Choose a reason for hiding this comment

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

Should we still consider this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this change makes sense - it fixes one problem, but there is now a new linking problem on Windows because we don't find the HDF5 library at link time. I haven't had time to figure out why.

@@ -47,7 +47,7 @@ jobs:
- name: Initial setup
shell: bash -l {0}
run: |
brew install eigen hdf5
brew install --force eigen hdf5

Choose a reason for hiding this comment

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

I think the new HDF5 comes with a new pkgconf, which is causing the linking issue.

We could consider running:

brew link --overwrite pkgconf
or
brew reinstall pkg-config

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know enough about brew to know in which order this would come relative to the lines we already have?

@ahnaf-tahmid-chowdhury
Copy link
Member

It appears that only HDF5 version 1.10.6 works for windows builds at the moment. The latest version, 1.14.4, doesn't include essential files like libhdf5.lib, among others, which might explain the issue. When I attempted to use version 1.12.2, I encountered the following error:

LINK : warning LNK4286: symbol '?find@Range@moab@@QEBA?AVconst_iterator@12@_K@Z (public: class moab::Range::const_iterator __cdecl moab::Range::find(unsigned __int64)const )' defined in 'Range.obj' is imported by 'Intx2Mesh.obj' [D:\a\dagmc-ci\moab_build\src\MOAB.vcxproj]
... (truncated warnings) ...
D:\a\dagmc-ci\moab_build\lib\Release\MOAB.dll : fatal error LNK1169: one or more multiply defined symbols found [D:\a\dagmc-ci\moab_build\src\MOAB.vcxproj]

For now, I’ve set the HDF5 version to 1.10.6. I assume, the MOAB library needs updates to properly support newer versions of HDF5 for Windows builds.

@ahnaf-tahmid-chowdhury
Copy link
Member

@gonuke, it seems yaml-cpp is a core dependency now. Please let me know how to address the gtest error.

@gonuke
Copy link
Member Author

gonuke commented Dec 4, 2024

I think this is failing for Windows on a very specific issue: CMake found the HDF5 libraries successfully during the configuration step, but they are not being found at the linking step. I am not sure how best to address this in Windows. I know there has been some substantial overhaul for Windows build in MOAB on the master branch, but switching to master didn't help.

@gonuke
Copy link
Member Author

gonuke commented Dec 4, 2024

As @ahnaf-tahmid-chowdhury points out, none of the 1.14.x conda-forge builds include libhdf5.lib files, but that appears to be intentional, so I wonder if we need to direct CMake to use different libraries?

@ahnaf-tahmid-chowdhury
Copy link
Member

I am not sure about this. I found that HDF5 1.14 does not include libhdf5.lib files. The HDF5 file found by CMake is actually hdf5.lib, which is smaller in size. For example, if you download and extract version 1.10.6, you will find both hdf5.lib (610KB) and libhdf5.lib (10.5MB), and in bin dir you will find hdf5.dll (3MB). However, in version 1.14.4, hdf5.lib (758 KB) and hdf5.dll (3.4MB) is available. I wonder if this is a bug on HDF5's side.

@ahnaf-tahmid-chowdhury
Copy link
Member

Is there any specific reason for using the conda version of HDF5 here? If we build it from source, I believe we may be able to overcome all the issues we are currently facing here.

@ahnaf-tahmid-chowdhury
Copy link
Member

ahnaf-tahmid-chowdhury commented Dec 4, 2024

Also, we should set our CMake version to at least 3.12 to enable <package>_ROOT. I noticed that these variables are being ignored during the configuration step because we have set the CMake version policy to 3.1. I recommend setting it to 3.18, which is also supported by ubuntu 20.04.

@gonuke
Copy link
Member Author

gonuke commented Dec 4, 2024

Is there any specific reason for using the conda version of HDF5 here? If we build it from source, I believe we may be able to overcome all the issues we are currently facing here.

If it's straightforward to build HDF5, we could certainly do that. What I'm concerned about is that building HDF5 for Windows launches us on a path for a complete overhaul of our build infrastructure.

@gonuke
Copy link
Member Author

gonuke commented Dec 4, 2024

I wonder if this is a bug on HDF5's side.

Seems unlikely since it's been a year since they included libhdf5.lib - so either no-one is using this for windows, or there's a way to only rely on hdf5.lib

@jon-proximafusion
Copy link

Perhaps building hdf5 from source is the only option then

@bam241
Copy link
Member

bam241 commented Dec 11, 2024

I wonder if this is a bug on HDF5's side.

Seems unlikely since it's been a year since they included libhdf5.lib - so either no-one is using this for windows, or there's a way to only rely on hdf5.lib

could we temporary fix this with a simple copy ? and have both name for the lib ?

This is not a good solution, but we could that way move forward :)

@gonuke
Copy link
Member Author

gonuke commented Dec 11, 2024

On older libraries, they are not the same file - they at least have different sizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants