Skip to content

Expand CI + pre-commit#991

Merged
j9ac9k merged 5 commits intopyqtgraph:developfrom
j9ac9k:expand-ci
Aug 29, 2019
Merged

Expand CI + pre-commit#991
j9ac9k merged 5 commits intopyqtgraph:developfrom
j9ac9k:expand-ci

Conversation

@j9ac9k
Copy link
Copy Markdown
Member

@j9ac9k j9ac9k commented Jul 10, 2019

Current azure-pipelines only does unit-testing on the 18 configurations we have set. With this pull request, azure-pipelines does more checks in line with the extra flag on used in Travis CI.

To be more specific, azure-pipelines now has multiple stages, pre-test and test. During the pre-test stage, the following things happen in parallel

  • python.setup.py style is run
    • checks flake8 required on entire codebase
    • checks flake8 optional on files that have diffs
    • checks for correct style line-endings
  • diff size is checked
  • pyqtgraph bdist_wheel is built (and then uploaded as an artifact)

During the test stage, all the unit tests run as before, with the exception that they download the bdist_wheel artifact and install that (so instead of creating 18 wheels and installing each one for on its own pipeline, we create one wheel, and use that wheel for all 18 pipelines)

The latest build can be seen here:

https://dev.azure.com/j9ac9k/pyqtgraph-fork/_build/results?buildId=284
In addition, this PR includes a few other features

  • .flake8 file is added to better support flake8 integration with editors and pre-commit
  • .pre-commit-config.yml is added allowing for support for pre-commit checks to run at commit time.

A note about pre-commit is added to CONTRIBUTING.md; it is strictly optional, however it will be helpful for ensuring code passes quality checks at commit time.

Also some changes were made to some files with diffs to make them conform to the new flake8 checks that are run against the CI.

@j9ac9k j9ac9k changed the title Expand CI + pre-commit + flake8 [WIP] Expand CI + pre-commit + flake8 Jul 11, 2019
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jul 11, 2019

Added [WIP] tag in an attempt to get the flake8 --diff checks going, not sure . that's going to be feasible though, will likely have to remove altogether.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jul 26, 2019

Started working on this on another branch, the flake8 -diff checks are still tough to integrate at the commit time. I am not a fan of having this check strictly in the CI (this takes too long to hear back that you have an extra space, or your line is too long).

I've created https://github.com/pyqtgraph/pre-commit-hooks and have been trying too make a hook to run the flake8 diff, but so far I haven't been able to get it working as I intended.

If anyone has any suggestions on how to integrate that check at commit time, let me know.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 16, 2019

ugh....what on earth is happening w/ creating conda environments in the CI process?

@j9ac9k j9ac9k force-pushed the expand-ci branch 7 times, most recently from d75dcf9 to 24cba27 Compare August 16, 2019 06:20
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 16, 2019

something has changed on azure pipelines with respect to how macOS and conda task work; this will take be a little while to debug.

@j9ac9k j9ac9k force-pushed the expand-ci branch 15 times, most recently from edd2300 to b1a4504 Compare August 16, 2019 20:27
@j9ac9k j9ac9k force-pushed the expand-ci branch 6 times, most recently from 1c232f7 to e2281b6 Compare August 16, 2019 22:25
@j9ac9k j9ac9k changed the title [WIP] Expand CI + pre-commit + flake8 Expand CI + pre-commit Aug 16, 2019
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 16, 2019

Gave up on trying to get linting checks to happen at the pre-commit time (doing a flake8 check on a file basis is fine, but doing it on the --diff I'm not sure can be done).

I had to modify the CI script substantially for a few reasons

  • conda 4.7.X has problems with python2 environments
    • fixed by manually downloading miniconda2 4.6.14 and installing it over the existing miniconda setup
  • -c conda-forge pyside on linux has some missing symbol issue that's an indication of a build related problem, but given its age, it won't be fixed (luckily -c defaults pyside` doesn't have this issue

@j9ac9k j9ac9k changed the title Expand CI + pre-commit [WIP] Expand CI + pre-commit Aug 18, 2019
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 18, 2019

Throwing WIP label as i realized i never actually tested the 100kb size limit on the size check...will want to do that before this is merged, I'll test on another branch

@j9ac9k j9ac9k changed the title [WIP] Expand CI + pre-commit Expand CI + pre-commit Aug 28, 2019
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 28, 2019

Cleaned up the PR, I think this is ready for review; only thing I couldn't do is get the flake8 --diff check to work at pre-commit time; oh well.

Copy link
Copy Markdown
Member

@campagnola campagnola left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks @j9ac9k !

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Aug 29, 2019

Thanks @campagnola I'll go ahead and merge as you gave approval

Thanks for everyone's patience with me on this,...this took me way too long to sort out, and there will likely be changes coming in the future with pytest-nunit, updates to pyqtgraph-core and doc builds.

@j9ac9k j9ac9k merged commit 584c451 into pyqtgraph:develop Aug 29, 2019
@j9ac9k j9ac9k deleted the expand-ci branch August 29, 2019 20:56
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.

3 participants