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

Cleanup around testing and packaging lifecycle #961

Closed

Conversation

psafont
Copy link
Contributor

@psafont psafont commented Nov 1, 2018

I've simplified managing with python packaging, removing test-requirements.txt and removing the direct usage of setup.py for installing the package and dependencies: that can be handled through pip and PEP518.

I also think there is a case to remove requirements.txt as it is, since it does not list all the dependencies needed in the environment, nor they are locked.

Let me know your thoughts, especially about requirements.txt. I haven't deleted the file but there is no other piece of code that actually uses it.

@codecov
Copy link

codecov bot commented Nov 2, 2018

Codecov Report

Merging #961 into master will increase coverage by 1.42%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage    79.2%   80.62%   +1.42%     
==========================================
  Files          30       30              
  Lines        5996     5884     -112     
  Branches     1488     1454      -34     
==========================================
- Hits         4749     4744       -5     
+ Misses        873      759     -114     
- Partials      374      381       +7
Impacted Files Coverage Δ
cwltool/update.py 37.73% <0%> (-30.19%) ⬇️
cwltool/process.py 83.07% <0%> (-3.03%) ⬇️
cwltool/argparser.py 83.88% <0%> (-1.9%) ⬇️
cwltool/builder.py 84.58% <0%> (-1.71%) ⬇️
cwltool/load_tool.py 83.16% <0%> (-1%) ⬇️
cwltool/pathmapper.py 83% <0%> (-0.97%) ⬇️
cwltool/executors.py 81.31% <0%> (-0.85%) ⬇️
cwltool/workflow.py 84.33% <0%> (-0.4%) ⬇️
cwltool/singularity.py 79.72% <0%> (-0.28%) ⬇️
cwltool/validate_js.py 85.71% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49cd284...cb48bec. Read the comment docs.

@psafont
Copy link
Contributor Author

psafont commented Nov 2, 2018

Python 3.4 is failing on appveyor because the wheel for 5.4.8 is not uploaded to PyPI: the compilation fails: giampaolo/psutil#1356

@psafont
Copy link
Contributor Author

psafont commented Nov 2, 2018

For Travis the failure appeared when enabling isolated builds (PEP518)

I've reported the issue to tox-travis, to try to see where the issue is really: tox-dev/tox-travis#119

tox.ini Show resolved Hide resolved
@mr-c
Copy link
Member

mr-c commented Nov 3, 2018

The travis failure is a manifestation of #954

@psafont psafont force-pushed the deduplicate_deps branch 9 times, most recently from d5607f6 to 618790c Compare November 5, 2018 11:07
@psafont
Copy link
Contributor Author

psafont commented Nov 5, 2018

I need to make sure the coverage reporting still works fine. After that the only issues there are is the (unrelated?) Travis failure, document the removal of support for python 3.4 on Windows, and removing requirements.txt.

After that is fixed there are a some thing that might be worth doing, maybe for another PR:

  • Activate isolated builds for tox when setuptools releases a version fixing the setup.py bug.
  • Clean jenkins' scripts, I'm not sure why coverage is used the way it's used, among other things.
  • Clean Makefile: is all that's there actually used?
  • Use this script to filter which environments are run on appveyor, otherwise tox detects all the python versions installed and executes all tests.

@psafont psafont changed the title WIP: Cleanup around python packaging Cleanup around testing and packaging lifecycle Nov 5, 2018
@psafont
Copy link
Contributor Author

psafont commented Nov 5, 2018

Coverage reporting seems to work find according to https://codecov.io/gh/common-workflow-language/cwltool/pull/961?src=pr&el=desc (80.43%, +2.23%)

@psafont psafont changed the title Cleanup around testing and packaging lifecycle WIP: Cleanup around testing and packaging lifecycle Nov 5, 2018
@psafont psafont self-assigned this Nov 5, 2018
@mr-c
Copy link
Member

mr-c commented Nov 5, 2018

Would including setup.py in MANIFEST.in be sufficient to fix the setuptools bug?

@psafont
Copy link
Contributor Author

psafont commented Nov 5, 2018

@mr-c looks like it does :)

About requirements.txt:
Snyk depends on requirements.txt to generate the analysis.

The main difficulty is that setup.py needs to be executed, opening up a possibility to execute malicious code, as it's executed on PRs.

The good solution would be to get rid of setup.py altogether (I'm testing migrating to poetry in another library)

"Importing" requirements.txt from setup.py is not possible since the project uses environment markers.

For now we probably should keep requirements.txt to keep the safety checks as migrating to poetry changes the packaging and release workflows. (testing and development should be unaffected as pip install . would use poetry as a backend)

@psafont psafont changed the title WIP: Cleanup around testing and packaging lifecycle Cleanup around testing and packaging lifecycle Nov 5, 2018
@psafont
Copy link
Contributor Author

psafont commented Nov 5, 2018

I also was thinking if we would want to delete of release-test.sh: the test of building the package in an isolated virtual environment and installing it in another is already being done with tox :)

Makefile Outdated Show resolved Hide resolved
@psafont
Copy link
Contributor Author

psafont commented Nov 22, 2018

Code coverage on codecov seems to be unaffected by parallellizing the tests, compared to #961 (comment)

remove test-requirements.txt
stop depending on requirements.txt
use python setup.py instead of ./setup.py whenever possible
Use PEP518 to automatically install setup dependencies
Reduce usage of coverage in favour of pytest-cov
tweaked dependency installing
simplified code for parallelizing tasks
added distclean
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