Remove tox dependency #882
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
A big positive is that we can now see the build logs, so we can find where the build is failing :) |
ssteinbach
force-pushed
the
remove_tox
branch
from
bce2df2 to
edff41c
Compare
- just seeing what pip does... if build messages still show up or not
Codecov Report
@@ Coverage Diff @@
## master #882 +/- ##
==========================================
+ Coverage 84.36% 92.49% +8.12%
==========================================
Files 74 9 -65
Lines 3090 2532 -558
==========================================
- Hits 2607 2342 -265
+ Misses 483 190 -293
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
- looks like pip doesn't have a way to only install dependencies, or to specify verbosity separate for the dependency search. Thats fine, CI will do a verbose install... better to get the build log than not.
- push even more configuration into the setup.cfg for the coverage runs - use the "path" argument to map the source paths to installed paths for opentimelineio and contrib modules - fix `make clean` target to clean up coverage files and not deal with .pyc files any more (since these are unlikely to be generated in the source tree)
- break out ci-prebuild and ci-postbuild commands, which ensures that the linter and check-manifest run _before_ builds and that coverage runs _after_ builds. - sets environment variables necessary for C++ coverage to be generated by ci builds
- the CMake was already testing this for C++ builds, so no need to duplicate the check. Instead, setup.py prints a hopefully helpful message about what is needed to enable C++ coverage builds. None of this impacts python coverage.
- check if lcov is installed for lcov target - add ci targets for before and after the install (ensuring that check-manifest and linting run before compiling and that testing/coverage runs after) - switch to using parallel mode for python coverage - add lcov target for C++ coverage report generation
- use the GITHUB_WORKSPACE variable from github actions to specify where the build directory lives
Please enter the commit message for your changes. Lines starting
- for ci builds, but might be helpful for home stuff too
Collaborator
Author
|
Making sure that the RTD build isn't busted. Lets wait for that to finish before landing this. |
Collaborator
Author
|
Those look good too, if this looks good to folks, I think this is ready to land: |
ssteinbach
commented
Feb 11, 2021
Collaborator
Author
|
Closes #772 |
meshula
reviewed
Feb 11, 2021
Collaborator
meshula
left a comment
meshula
left a comment
There was a problem hiding this comment.
There's one chunk to remove, as you asked. The CMake side of things already has logic to disable coverage if lcov is not available on the platform (e.g. msvc)
- add a brief description of how to generate C++ coverage enabled builds - updated the supported python version in the README (and hopefully used a string that will be easier to find when updating the README in the future).
meshula
approved these changes
Feb 11, 2021
meshula
approved these changes
Feb 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've used
toxfor some time, and its served us well. With the recent move to GitHub actions, however, we've found that it is no longer needed. The reasons for removing it:tox, so this ensures that developers who use our developer makefile are matching exactly what the ci doesWhat this PR does:
toxwas running into a makefile target,ci-build-testtoxtoxfrom various places (mostly thesetup.cfg).Todo:
lcovexecution toMakefiletoxsection ofsetup.cfgsetup.pyto see if we can remove extra logic whose comment references tox