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

Enable double-down in CI for pull requests. #738

Merged
merged 5 commits into from
Jun 24, 2021
Merged

Conversation

pshriwise
Copy link
Member

Seeing as this option is being increasingly used outside of DAGMC, I'd like to add it to CI run with pull requests to make sure things aren't breaking as development continues.

@pshriwise
Copy link
Member Author

I'm seeing failures in CI, but they don't seem to be related to the changes here. Am I interpreting that right @bam241?

@bam241
Copy link
Member

bam241 commented Apr 19, 2021

I am 90% sure that comes from make -j${ci_procs} for some reason gcc is not stable when using to many proc to compile....

@pshriwise
Copy link
Member Author

Ah I see. Should I try reducing the number of make jobs here or is that being addressed in another PR?

@gonuke
Copy link
Member

gonuke commented Apr 19, 2021

This is clashing a little with the fixes I have in #687, but I thought the fix for GCC already existed in the env.sh that should help this PR as well

@gonuke
Copy link
Member

gonuke commented Apr 19, 2021

P.S. #687 is basically waiting on the race condition fix for MOAB because that's causing the tests to fail

@pshriwise pshriwise added build & CI DONT_REVIEW_YET Not yet ready for review. Please don't waste your time :) labels Apr 19, 2021
@pshriwise
Copy link
Member Author

Happy to let #687 go in first. I can handle any conflicts that come up. Added some labels here to make sure we wait a bit on this.

P.S. #687 is basically waiting on the race condition fix for MOAB because that's causing the tests to fail

That PR should go in soon -- the ball is in the MOAB dev team's court to add a change to my PR before we merge it. It won't fix our broken builds for MAOB 5.1.0 though. Do we have a plan for how to deal with that going forward? Update to the most recent release of MOAB with that fix and rely on it for CI? Should we also disable the overlap_check program or limit it to a single thread depending on the MOAB version?

@bam241
Copy link
Member

bam241 commented Apr 20, 2021

This is clashing a little with the fixes I have in #687, but I thought the fix for GCC already existed in the env.sh that should help this PR as well

we changed the variable name to ${ci_proc} in order to avoid conflict with the CMake var ${proc}. I am unsure if this solved anything with regard to gcc errors....
I usually experience this kind of gcc error with I compile with a very high -jX or with only the -j

@bam241
Copy link
Member

bam241 commented Apr 20, 2021

@gonuke
Copy link
Member

gonuke commented Apr 20, 2021

But we also have a check that sets ${ci_proc} to be only 4 for bcc

@gonuke
Copy link
Member

gonuke commented Jun 22, 2021

Should this be matrixed in to the other builds?

@pshriwise
Copy link
Member Author

pshriwise commented Jun 22, 2021

Should this be matrixed in to the other builds?

If it won't affect the CI runtime too much, I'd be happy to add it to the job matrix. What do you think @bam241?

@bam241
Copy link
Member

bam241 commented Jun 22, 2021

sounds like a good plan

@gonuke
Copy link
Member

gonuke commented Jun 22, 2021

On second thoughts... maybe leave it out for now. We probably need to rethink CI as the number of dimensions of the matrix expand. We probably don't need to fill the matrix anyway, and we might want to move from 16.04 to 18.04.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Let's merge this and worry about the CI matrix at another time.

@gonuke gonuke merged commit 54cc729 into svalinn:develop Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build & CI DONT_REVIEW_YET Not yet ready for review. Please don't waste your time :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants