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

Update the World builder to version 0.6 #5584

Merged
merged 5 commits into from Mar 27, 2024

Conversation

MFraters
Copy link
Member

A new version of the world builder will be released soon (hopefully, see GeodynamicWorldBuilder/WorldBuilder#670 for todo's), so it is also time to start the review process to get it into aspect. This pull request will still be update until the final release.

I know @tjhei and @gassmoeller looked into improving the cmake system for building, but I don't know how that discussion ended. Is the current form oke, or are changes needed? Note that how the cmake currently works in aspect is that is gwb version >= 0.6, it will use the new system.

@tjhei
Copy link
Member

tjhei commented Feb 25, 2024

I have a bunch of changes on ASPECT and GWB that are needed to fix the DebugRelease build. Needs cleaning up first, though.

@MFraters MFraters force-pushed the update_world_builder_to_0.6 branch 2 times, most recently from 3d1c414 to c80e028 Compare March 7, 2024 19:37
@MFraters MFraters changed the title [WIP] Update the World builder to version 0.6 [Review ready, Do not merge] Update the World builder to version 0.6 Mar 7, 2024
@MFraters MFraters force-pushed the update_world_builder_to_0.6 branch 3 times, most recently from 97bb2b5 to 21d8961 Compare March 7, 2024 20:07
@MFraters
Copy link
Member Author

MFraters commented Mar 7, 2024

This pull request is now ready for review. Before making the world builder 0.6 release, I would like preliminary approval on this pull request that this is in principle good to merge.

Once I have that, I will remove the -pre from the version numbers and add the new zenodo doi and make the release. When that is done, I will update this pull request again with the released version.

@gassmoeller
Copy link
Member

This time the tester errors are a little more clear. Seems like something goes wrong in DebugRelease mode with linking to WB. Have you checked if this works locally for you?

@MFraters MFraters force-pushed the update_world_builder_to_0.6 branch 2 times, most recently from c2b4898 to 61e3b4c Compare March 7, 2024 21:46
@MFraters
Copy link
Member Author

MFraters commented Mar 7, 2024

Fixed it. It looks good for review now :)

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didnt look at the content of the world builder directory, but that will only be a copy of the world builder release eventually, right? The changes to ASPECT's CMakeLists look ok.

@MFraters
Copy link
Member Author

MFraters commented Mar 8, 2024

Looks good to me. I didnt look at the content of the world builder directory, but that will only be a copy of the world builder release eventually, right? The changes to ASPECT's CMakeLists look ok.

Thanks. Yes, that that is correct.

@MFraters MFraters force-pushed the update_world_builder_to_0.6 branch from 61e3b4c to 03f7aca Compare March 9, 2024 00:16
@MFraters MFraters changed the title [Review ready, Do not merge] Update the World builder to version 0.6 Update the World builder to version 0.6 Mar 9, 2024
@MFraters
Copy link
Member Author

MFraters commented Mar 9, 2024

The world builder 0.6.0 release has been made and this pull request is updated with the released version.

@tjhei
Copy link
Member

tjhei commented Mar 11, 2024

Why does ci.tjhei.info fail?

@tjhei
Copy link
Member

tjhei commented Mar 11, 2024

ld: file not found: -lWorldBuilder

This looks like some of the CMake changes I made did not work. Do you want me to take a closer look?

@MFraters
Copy link
Member Author

oke, I had to experiment a bit, because I couldn't reproduce it locally, but it seems fixed now.

@MFraters
Copy link
Member Author

Is there anything which still need to happen here? I think it now works as intended and it should be ready to merge.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

I think this is good to go, we probably just forgot that the PR is still open.

@gassmoeller gassmoeller merged commit ee9b454 into geodynamics:main Mar 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants