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

Add dealii dev tester #4099

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

gassmoeller
Copy link
Member

This adds a github action that compiles ASPECT against the dealii/dealii:master-focal container with every PR. Currently does not run the test suite.

No more PRs that work on the tester but break on deal.II dev 🎉

@gassmoeller
Copy link
Member Author

Rebased on #4100.

@gassmoeller
Copy link
Member Author

I rebased on #4100, this should be ready if we agree it is a good idea.

@tjhei
Copy link
Member

tjhei commented Jun 30, 2021

Is it useful to do this on each PR? I think we talked about it to be run on master at regular intervals instead. It is a bit weird that a change of deal.ii master will make the next user PR fail...

@gassmoeller
Copy link
Member Author

I am a bit undecided. Yes it is weird that deal.II dev would make our tester fail, on the other hand it ensures high priority. If we run it daily instead we can actually run the full test suite (without comparing results), but we would still merge PRs that break with deal.II dev. Not sure which (or both) is better, what do you think?

@tjhei
Copy link
Member

tjhei commented Jun 30, 2021

Not sure either. I would say it is more likely to have deal.ii master change, than a user adding something that works with the last release but not with master.

@gassmoeller
Copy link
Member Author

Alright, I will change it to run daily on master.

@bangerth
Copy link
Contributor

bangerth commented Jul 1, 2021

Whatever breaks with deal.II dev at any given moment will require a fix sooner rather than later anyway. We might as well catch this at the time of proposing a patch, and if necessary spend a few minutes explaining the situation to someone, rather than requiring someone else to go back to fix it.

@bangerth
Copy link
Contributor

bangerth commented Jul 1, 2021

In other words, I'm for it.

@gassmoeller
Copy link
Member Author

@bangerth: Do you mean you are for a daily run of the full testsuite (would require changes), or a smaller run for every PR that only compiles (the current state of this PR)? I am fine with either or both.

@bangerth
Copy link
Contributor

bangerth commented Jul 2, 2021

If we have the resources, I'm always for adding things to the CI :-)

@gassmoeller
Copy link
Member Author

In that case this PR is ready for merge (this is the quick 10 min test that aspect compiles with deal.II dev). I agree that it puts more pressure on us to fix problems quickly. Having a daily test that just sends an email, or changes a badge color is not as obvious. If it triggers too often because of unrelated things we can always disable it later.

@bangerth
Copy link
Contributor

bangerth commented Jul 6, 2021

I'm in favor. Should we merge?

@gassmoeller
Copy link
Member Author

Yes.

@tjhei
Copy link
Member

tjhei commented Jul 6, 2021

I am not convinced that this won't end up confusing PR creators, but we can try...

@bangerth
Copy link
Contributor

bangerth commented Jul 7, 2021

Looks like we need a bold leader willing to take a stand and press the button, consequences be damned! I will be that person for today!

@bangerth bangerth merged commit bf2ed46 into geodynamics:master Jul 7, 2021
@gassmoeller gassmoeller deleted the add_dealii_dev_tester branch July 7, 2021 01:52
@ljhwang
Copy link
Contributor

ljhwang commented Jul 7, 2021

Was that Gru I saw on your desktop?

@bangerth
Copy link
Contributor

bangerth commented Jul 7, 2021

I don't know who Gru is, but I had the minion, the superhero, and a bunch of dinos from last year that have kept me company since then :-)

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.

4 participants