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

Reduce number of systems test with minimal config #36941

Closed
wants to merge 5 commits into from

Conversation

tobiasdiez
Copy link
Contributor

Alternative solution for #36726. Since the purpose of the "minimal" ci runs is to check the documentation of the minimal build requirements, it is overkill to run it for all systems. Thus, we only run it for "ubuntu-focal", which is also the default system for PRs.

This also hugely reduces the load of onto the ci, somewhat leviating the issues introduced by #36616 (see eg #36616 (comment)).

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@mkoeppe
Copy link
Member

mkoeppe commented Dec 21, 2023

This is just vandalism.

@dimpase
Copy link
Member

dimpase commented Dec 21, 2023

no, your minimal configurations are vandalism.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 21, 2023

no, your minimal configurations are vandalism.

@dimpase How so?

#36726 (comment)

@dimpase
Copy link
Member

dimpase commented Dec 21, 2023

cause it's vandalism to test on CI 1000s times something big and stable, like building a particular version of gcc/gfortran, on the same systems - on which this gcc/gfortran is absolutely not needed.

This is basically burning money.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 21, 2023

What you are describing does not fit the definition of "vandalism" at all.

I'll take it as a reflexive reaction, a "no, you are" comeback.

@dimpase
Copy link
Member

dimpase commented Dec 22, 2023

well, pick up your favourite term for doing meaningless, time-consuming, and potentially expensive stuff, and forcing other people to help you with it.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 22, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Alternative solution for sagemath#36726. Since the purpose of the "minimal" ci
runs is to check the documentation of the minimal build requirements, it
is overkill to run it for all systems. Thus, we only run it for "ubuntu-
focal", which is also the default system for PRs.

This also hugely reduces the load of onto the ci, somewhat leviating the
issues introduced by sagemath#36616 (see eg
sagemath#36616 (comment)).

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36941
Reported by: Tobias Diez
Reviewer(s): Dima Pasechnik
@mkoeppe
Copy link
Member

mkoeppe commented Dec 22, 2023

@vbraun Please back this out

@mkoeppe
Copy link
Member

mkoeppe commented Dec 22, 2023

well, pick up your favourite term for doing meaningless, time-consuming, and potentially expensive stuff, and forcing other people to help you with it.

@dimpase I called you out clearly for making such denunciations, which are clear CoC violations. #36726 (comment)
How do you justify this continued misconduct?

@dimpase dimpase added s: needs info disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed r: invalid s: needs review labels Dec 22, 2023
@dimpase
Copy link
Member

dimpase commented Dec 23, 2023

@mkoeppe - have you read @jhpalmieri 's email?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 23, 2023

@dimpase Yes. How do you justify your continued severe misconduct?

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 23, 2023
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Alternative solution for sagemath#36726. Since the purpose of the "minimal" ci
runs is to check the documentation of the minimal build requirements, it
is overkill to run it for all systems. Thus, we only run it for "ubuntu-
focal", which is also the default system for PRs.

This also hugely reduces the load of onto the ci, somewhat leviating the
issues introduced by sagemath#36616 (see eg
sagemath#36616 (comment)).

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

URL: sagemath#36941
Reported by: Tobias Diez
Reviewer(s): Dima Pasechnik
Copy link

Documentation preview for this PR (built with commit 72b72cd; changes) is ready! 🎉

@roed314
Copy link
Contributor

roed314 commented Mar 12, 2024

This PR is an appropriate reaction to #36726. It removes CI testing for all Linux distros except one version of Ubuntu (which happens to have pkill).  It is an extreme solution to a problem that is already solved.   Reducing testing to only one distro is something that should obviously be discussed and voted on sage-devel. A civil discussion on sage-devel of the pros and cons of different approaches to CI testing of sage would be valuable; this is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: scripts disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ r: invalid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants