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 CI via Github actions #68

Merged
merged 2 commits into from
May 22, 2020
Merged

Add CI via Github actions #68

merged 2 commits into from
May 22, 2020

Conversation

Flamefire
Copy link
Contributor

This adds CI tests via Github actions where it installs scorep and this module and runs test/test.py

It also checks formatting (as per clang-format-9) and for Python issues (via flake8)

See commits for details

@Flamefire Flamefire changed the title Add CI Add CI via Github actions May 20, 2020
@AndreasGocht
Copy link
Collaborator

You are changing way too much infrastructure in one PR. I am not going to merge this as it is.

About the CI system:

Having two CI systems just unnecessary overhead. What are the benefits of Github actions above Travis?

About Code Formating:

What is the benefit of flake8 above autopep8?
Please see the other comments inline.

About .gitignore:

I do not like .gitignore files in the repo. This requires me to add the files I need to ignore to the repo (e.g. from eclipse), which ends up in a total mess of files if any developer does this. Moreover, adding e.g. /scorep.egg-info will prevent git clean -df from working "correctly", and removing all build files.

@Flamefire Flamefire mentioned this pull request May 20, 2020
@Flamefire
Copy link
Contributor Author

I moved the .gitignore into #70, the remaining are: CI with linting and make it pass (fix issues detected by that CI). IMO those belong together but I can split this further if you tell me how you'd like your PRs.

What are the benefits of Github actions above Travis?

Travis needs to be set up, GHA is enabled for all repos by default (it seems it was explicitly disabled here, or the PR must be accepted first though), especially useful for running the tests in branches before opening a PR.
Additionally it is usually faster (out of experience) and IMO easier to set up. See how the different tasks are split in properly named files and subtasks. Also each job will show up in a PR status directly showing which one failed.

What is the benefit of flake8 above autopep8?

autopep8 changes code according to errors reported by flake8. So it is a formatter while flake8 is a linter. So you can't compare them.
If your question was why autopep8 isn't run and then checked for changes: It cannot fix all issues flake8 detects, while it can only fix issues detected by flake8. Hence flake8 is required for completeness.

@AndreasGocht
Copy link
Collaborator

I moved the .gitignore into #70,

Thanks.

the remaining are: CI with linting and make it pass (fix issues detected by that CI). IMO those belong together but I can split this further if you tell me how you'd like your PRs.

I'd like to squash and merge the PR's. So I think it's a good idea to split this further. What about:

  • fix the linting
  • set the CI in place

CI System

Travis needs to be set up, GHA is enabled for all repos by default (it seems it was explicitly disabled here, or the PR must be accepted first though), especially useful for running the tests in branches before opening a PR.

As far as I remember I did not explicitly disable it but never used it. Github CI cam after I started to use Travis. Branches and PR's are done by Travis as well, and as stated, was there before.

Additionally it is usually faster (out of experience) and IMO easier to set up.

Especially the setup is a question of what are you used to.

See how the different tasks are split in properly named files and subtasks.

For me, this looks quite chaotic 😉

Also each job will show up in a PR status directly showing which one failed.

This seems to be a good reason. Please draft a separate pull request for the CI, to discuss the details.

Code formating

autopep8 changes code according to errors reported by flake8. So it is a formatter while flake8 is a linter. So you can't compare them.

Ok, I wasn't aware of the difference. As already stated, please do a sperate PR. I will merge this first, and the CI one later. Please also state the errors, that are detected by flake8

@Flamefire
Copy link
Contributor Author

As far as I remember I did not explicitly disable it but never used it

Doublecheck "Settings"->"Actions"->"Enable local and third party Actions for this repository", but it is possible, that it is automatically enabled once any action is defined in the repo, i.e. after this is merged

Branches and PR's are done by Travis as well, and as stated, was there before.

I mean: In my fork I'd have to set up travis myself before branches are build, this (minor) work isn't required for GHA.

For me, this looks quite chaotic

Give it a chance. Especially the matrix feature is better than all others as you can have different matrices which I found lacking in other CIs for more complex test workflows.

Anyway, rebased this PR removing all unrelated commits and moved those to separate PRs. Try merging this first, and observe the failures by the static_analysis job (view in the "actions" tab above)
I can add a badge to the readme if you want

Copy link
Collaborator

@AndreasGocht AndreasGocht left a comment

Choose a reason for hiding this comment

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

Especially the matrix feature is better [...]

interesting. I do like it somehow. However, there are two questions left. Once they are clarified we can merge this.

.github/workflows/static_analysis.yml Show resolved Hide resolved
.github/workflows/unit_tests.yml Show resolved Hide resolved
@AndreasGocht
Copy link
Collaborator

I can add a badge to the readme if you want

yes, please. At best as part of this PR.

@Flamefire
Copy link
Contributor Author

Flamefire commented May 20, 2020

Added the bages with links to the would-be status pages. Flying blind here as they haven't run yet so got to recheck once this is merged

@AndreasGocht AndreasGocht merged commit eb3299a into score-p:master May 22, 2020
@AndreasGocht
Copy link
Collaborator

Thanks.

@Flamefire Flamefire deleted the ci branch May 22, 2020 12:34
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.

2 participants