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 missing includes with the help of inshpect #1129

Merged
merged 63 commits into from
May 24, 2024

Conversation

gulivarese
Copy link
Collaborator

No description provided.

@gulivarese gulivarese self-assigned this Apr 30, 2024
@gulivarese
Copy link
Collaborator Author

cscs-ci run

.inshpect.toml Outdated Show resolved Hide resolved
@msimberg
Copy link
Collaborator

Reminder, we should make the inshpect check required before merging this.

@msimberg msimberg changed the title Add include with the help of inshpect Add missing includes with the help of inshpect May 21, 2024
Copy link
Collaborator

@rasolca rasolca left a comment

Choose a reason for hiding this comment

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

Is it normal that the check takes so long?
Currently 1h30 and still running.

include/dlaf/matrix/print_csv.h Outdated Show resolved Hide resolved
@msimberg
Copy link
Collaborator

Is it normal that the check takes so long? Currently 1h30 and still running.

Certainly not... It should take a couple of minutes at most. I can't seem to cancel the runs either (and check_format also seems to be stuck...). We may have to wait for the jobs to time out (hopefully) and retry. Looks like it may perhaps be an issue with the runners.

@rasolca
Copy link
Collaborator

rasolca commented May 21, 2024

Is it normal that the check takes so long? Currently 1h30 and still running.

Certainly not... It should take a couple of minutes at most. I can't seem to cancel the runs either (and check_format also seems to be stuck...). We may have to wait for the jobs to time out (hopefully) and retry. Looks like it may perhaps be an issue with the runners.

Apparently something is wrong with actions... even #1146 is having problems.


jobs:
inshpect:
runs-on: ubuntu-22.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to run it already on ubuntu 24.04

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect, I add this review!
Thanks @rasolca !

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a very good point @rasolca. @gulivarese this means that you can now install dasel and ripgrep directly with apt. dasel was not yet available in 22.04, and ripgrep was not compiled with PCRE2 in 22.04, so they both had to be installed manually with 22.04. With 24.04 both of these are fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@msimberg so now I need to runs-on: ubuntu-22.04 and then I can delete the two steps below about the installation of the two dependencies (ripgrep and dasel).
Is it correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need runs-on: ubuntu-24.04 (note 24, not 22). You of course need to install them with apt instead and since I haven't tried it myself the ubuntu versions may not be compatible, but give it a try and then we'll see if it works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sorry I made an mistake when I wrote 22.04

I pushed my modify and my updates.

@rasolca
Copy link
Collaborator

rasolca commented May 23, 2024

cscs-ci run

@rasolca rasolca merged commit 3db7fc8 into eth-cscs:master May 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants