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

Changing AEGIS recipe for Sonoma build #5191

Closed
wants to merge 1 commit into from

Conversation

ChSonnabend
Copy link

Modifying dependencies of

  • boost
  • dds
  • aegis
    for a successful build on macOS Sonoma

@ChSonnabend
Copy link
Author

Tagging @TimoWilken

@TimoWilken
Copy link
Contributor

Thank you very much @ChSonnabend! I'll mark the PR as "ready for review" so that the CI will run.

@TimoWilken TimoWilken marked this pull request as ready for review October 27, 2023 11:21
@TimoWilken TimoWilken requested review from a team as code owners October 27, 2023 11:21
TimoWilken
TimoWilken previously approved these changes Oct 27, 2023
Copy link
Contributor

@TimoWilken TimoWilken left a comment

Choose a reason for hiding this comment

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

Approving for CI

@@ -21,7 +21,8 @@ if [ $FVERSION -ge 10 ]; then
echo "Fortran version $FVERSION"
SPECIALFFLAGS=1
fi
cmake $SOURCEDIR -DCMAKE_INSTALL_PREFIX=$INSTALLROOT \
cmake $SOURCEDIR -DCMAKE_CXX_FLAGS=-isystem\ "/opt/homebrew/include" \
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to achieve here?

Copy link
Author

Choose a reason for hiding this comment

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

This was needed to find the system dependencies for nlohmann/json. Not sure if this is needed in general, but I added it here for now.

Copy link
Member

Choose a reason for hiding this comment

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

If this is some "out-of-band" external, we should probably have a separate recipe for it.

boost.sh Outdated
version: v1.75.0
tag: v1.75.0
version: v1.82.0
tag: v1.82.0
Copy link
Member

Choose a reason for hiding this comment

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

We have a separate PR for this (#5140), however it breaks running multiple O2 tests in parallel. Until that is understood and solved, we cannot merge it.

Copy link
Author

Choose a reason for hiding this comment

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

Ah ok, this I didn't know. For me it works on Sonoma (and it works only like this, not with the older version...).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it compiles and runs fine with 1.82.0 as long as you run one workflow at the time. When you run more than one, sometimes it fails. You can find more details in the error report of the other PR.

dds.sh Outdated
@@ -1,6 +1,6 @@
package: DDS
version: "%(tag_basename)s"
tag: "3.7.22"
tag: "3.7.23"
Copy link
Member

Choose a reason for hiding this comment

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

This is good to go already, so I split it in a different PR (#5192).

Copy link
Member

@ktf ktf left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I have a few comments inline. I would use the orginal PR for the boost changes. Moreover, the aegis changes cannot be merged as they are, since they hardcode a global system path in the search. If you really do not want to package json for now, you should at least use $(brew --prefix json)/include and have it as part of a macOS specific flag.

@ChSonnabend ChSonnabend changed the title Adding three dependencies for building on Sonoma Changing AEGIS recipe for Sonoma build Oct 30, 2023
@ChSonnabend
Copy link
Author

After some exchanges with @ktf only the AEGIS changes were integrated in this PR.

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.

None yet

3 participants