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

Pr from fork/1376 (Sina as Axom component) #1378

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

Conversation

BradWhitlock
Copy link
Member

@BradWhitlock BradWhitlock commented Jul 8, 2024

Summary

This PR is kind of a mirror PR for #1376 (Sina as an Axom component) that allows Axom's CI tests to run. The procedure for forked repo contributions was followed.

Note that this PR must be approved as it will be merged.

Also note that I activated the generated docs for Sina on RTD so they can be reviewed for formatting, etc. Here is the link: https://axom.readthedocs.io/en/pr-from-fork-1376/axom/sina/docs/sphinx/index.html

@rhornung67
Copy link
Member

@bgunnar5 I added you to the axom GitHub team so you should be able to make PRs, etc. I tagged @LLNL/axom team to continue to review your PR here. The associated branch is in the axom repo made from your fork (sorry @BradWhitlock since you created this PR, I can't tag you as a reviewer). @bgunnar5 this is the PR that will ultimately be merged, not the one you created on your fork. We can explain why we have to do this at the axom meeting next Monday. Anyway, please try checking out this PR branch and merge everything on your fork branch into this. That way you can continue to make change based on the PR reviews here and we can avoid tracking two branches and PRs. If you run into any issues trying what I suggested, please let us know. Thank you for your work on this.

@bgunnar5
Copy link
Member

does anyone know if there is a simple way to push changes directly to this branch from my forked branch? I tried adding this repo as an upstream remote, fetching from it, then git pushing directly here but git doesn't seem to like that very much.

@white238
Copy link
Member

You could push this branch after changing your origin to the main repo now that you have access to it.

@bgunnar5
Copy link
Member

For future reference: looks like if you have this repo set as an upstream remote you can run git push upstream HEAD:<pr branch>. Apparently the HEAD part is key to get this to work.

@bgunnar5
Copy link
Member

@LLNL/axom I'm investigating test failures right now and it looks like most of the issues come from the blueos system, specifically because of the use of MATCHER_P from gmock. It looks like Klee also had this problem in the past:

// Note: After updating to the GMock in [email protected], the clang+nvcc compiler on LLNL's blueos

Do we know if there is an easy solution for this yet? If not I can just create my own matcher for Sina like Klee does, but figured I'd check first before going down that rabbit hole.

@rhornung67
Copy link
Member

@bgunnar5 it seems odd that everything works for gcc, but not for gcc+cuda since it looks like the compilation error originates in code that is not for a GPU. Also, it looks like some sort of argument type mis-match. Does it make sense to cast the args passed to the matcher macro to types that work?

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

4 participants