MNT: Update doc about setup.py and py.test #10756

pllim · 2020-09-22T14:15:06Z

Description

This pull request is to address mentions of outdated setup.py and py.test usage.

I am still not so sure about milestoning doc changes. I am guessing this should go in bug fix for 4.1? If not, feel free to re-milestone.

Close #10752

p.s. There was also a previous effort to replace py.test in #10722, which seemed to have missed the files I edited here.

saimn

Looks good, just a few comments.

saimn · 2020-09-22T15:32:00Z

astropy/tests/runner.py

@@ -334,7 +334,7 @@ def coverage(self, coverage, kwargs):
            warnings.warn(
                "The coverage option is ignored on run_tests, since it "
                "can not be made to work in that context.  Use "
-                "'python setup.py test --coverage' instead.",
+                "'pytest --coverage' instead.",


--coverage is an option specific to setup.py test, and since this is a warning specific to the test runner it should not be changed ?

But we don't allow setup.py test anymore. I am confused.

yeah I think this should be pytest --cov here not --coverage

@saimn made me undo the changes, so --cov doesn't apply no more.

This warning is emitted when you run setup.py test, so it seems weird that it then refers to pytest instead.
And I'm not sure if setup.py test is actually deprecated ?

It is not deprecated, it outright won't work anymore as far as this package is concerned.

astropy/setup.py

Line 13 in 7962503

Note: running tests is no longer done using 'python setup.py test'. Instead

The test command is still used by downstream packages that use astropy helpers

saimn · 2020-09-22T15:32:39Z

astropy/tests/runner.py

@@ -378,7 +378,7 @@ def test_path(self, test_path, kwargs):

            if ext in ('.rst', ''):
                if kwargs['docs_path'] is None:
-                    # This shouldn't happen from "python setup.py test"
+                    # This shouldn't happen from "pytest"


saimn · 2020-09-22T15:34:42Z

docs/development/astropy-package-template.rst

@@ -124,7 +127,7 @@ by ``CHANGES.md`` in the instructions.
   all pass, you can proceed on.

 #. If you did the previous step, do ``git clean -fxd`` again to remove anything
-   you made there.  Run ``python setup.py build sdist --format=gztar`` to
+   you made there.  Run ``python -m pep517.build --source .`` to


This will need to be replaced by python-build in the short term (https://python-build.readthedocs.io/en/latest/). I'm not sure exactly what's the status of this, but pep517.build is going to be replaced by python-build soon.

Until python-build is shipped officially everywhere, we cannot recommend it yet.

Yes, there's a bit of urgency to make pep517.build disappear, see discussion pypa/pyproject-hooks#83

If this bit of documentation is going to stick around in a stable version for some months, I'd say it's risky

You can install python-build already, so I don't think there is an issue with recommending it now.

Agree we should mention python-build here

But why? Isn't it too soon? I tried using it in one of my packages and it doesn't work.

all python build is, is a CLI for pep517 it should work fine. What trouble did you run into?

My trouble: https://dev.azure.com/spacetelescope/acstools/_build/results?buildId=3756&view=logs&j=b25f120e-e38e-5d50-120e-fa71f5e29986&t=b1d60ffd-f6c2-56b4-1446-ab1e65159582&l=71

The diff: spacetelescope/acstools@f556c6f

Am I using it wrong? I tried to follow the Python doc.

You didn't install the build package 😉 Also, have you considered using https://github.com/OpenAstronomy/azure-pipelines-templates I hear some really awesome people maintain it 😛

Huh... You would think Python would install it automatically since it is like so essential. Thanks! I'll try that.

Re: Azure -- I would have just switched to Actions. I just don't have time to refactor the CI for that package. 😬

docs/development/workflow/command_history.sh

pllim · 2020-09-22T18:27:46Z

@saimn , I think I have addressed your comments. How does it look now?

astrofrog

Wow so much outdated stuff, thanks for catching it all! A few comments below.

astrofrog · 2020-09-23T10:41:33Z

astropy/tests/command.py

-``./setup.py test`` distutils command.
-"""
-
+"""Implements the wrapper for the Astropy test runner."""


Maybe add a note that this is just here for backward-compatibility for other packages and can be removed once astropy-helpers is no longer supported?

astrofrog · 2020-09-23T10:43:30Z

docs/development/astropy-package-template.rst

@@ -57,6 +57,9 @@ by ``CHANGES.md`` in the instructions.

        python -m pep517.build --source .

+   All following instructions will assume you have ``pyproject.toml``, so
+   please adjust accordingly if that is not the case yet.


What about linking to the 4.0 docs for projects that don't have this file?

astrofrog · 2020-09-23T10:44:14Z

docs/development/astropy-package-template.rst

@@ -124,7 +127,7 @@ by ``CHANGES.md`` in the instructions.
   all pass, you can proceed on.

 #. If you did the previous step, do ``git clean -fxd`` again to remove anything
-   you made there.  Run ``python setup.py build sdist --format=gztar`` to
+   you made there.  Run ``python -m pep517.build --source .`` to


Agree we should mention python-build here

astrofrog · 2020-09-23T10:45:20Z

docs/development/workflow/command_history.sh

 #
 # Check setup by running tests
 #
 cd astropy/coordinates/tests    # get ready to run coordinate tests
-py.test                         # test
+pytest                          # test


Instead of these two lines, pytest astropy/coordinates would be better because it will load the pytest config in setup.cfg and will also run doctests not in the tests directory.

astrofrog · 2020-09-23T10:45:43Z

docs/development/workflow/command_history.sh

 ls                              # what is here?
 #
 # Write a test in test_arrays.py to expose this bug
 #
 # After edit, re-test
 #
-py.test test_arrays.py          # Hopefully this succeeds
+pytest test_arrays.py           # Hopefully this succeeds


pytest astropy/coordinates/tests/test_arrays - etc for the rest of the instructions

I think updating the actual instruction is out of scope here. The whole thing is way outdated. The log shows Python 2, etc etc. Maybe @mwcraig can update it when he has time. I am not even sure where this info is displayed. I just modify it for consistency in regards to "py.test" since grep found it.

I had completely forgotten about this. It probably needs to be re-written from the ground up if we want it to stay around. I agree this is outside the scope of this PR.

Okay, we can follow-up over at #10777 . Thanks!

astrofrog · 2020-09-23T10:46:14Z

docs/development/workflow/command_history_with_output.sh

@@ -305,7 +303,7 @@ accuracy/test_icrs_fk5.py .

 # So far, so good, now check ALL tests
 $ cd ../../..                       # up to the top level to check all of the tests


Again we should basically always run pytest from the root of the repo

pllim · 2020-09-23T13:31:14Z

I have applied the comments except the following:

~~python-build (see convo above).~~
fixing stale shell scripts -- out of scope

pllim · 2020-09-23T15:52:22Z

Okay, what about now?

saimn

Looks good to me, thanks @pllim 👍
I will leave the final approval to @astrofrog.

astrofrog

Looks good to me!

MNT: Update doc about setup.py and py.test

bsipocz · 2020-11-25T05:18:56Z

This has been already released, but milestone hasn't been updated accordingly, fixing it now.

pllim added Docs no-changelog-entry-needed labels Sep 22, 2020

pllim added this to the v4.1.1 milestone Sep 22, 2020

pllim requested review from astrofrog and Cadair September 22, 2020 14:15

pllim mentioned this pull request Sep 22, 2020

Downstream cloud install failing (setup.py install does not install astropy wheel) #10752

Closed

Cadair approved these changes Sep 22, 2020

View reviewed changes

pllim added the Ready-for-final-review label Sep 22, 2020

saimn reviewed Sep 22, 2020

View reviewed changes

pllim added 2 commits September 22, 2020 14:12

MNT: Update doc about setup.py and py.test

a773437

Address review comments from Simon Conseil

13aa984

pllim force-pushed the no-setup-py branch from 554c708 to 13aa984 Compare September 22, 2020 18:20

astrofrog requested changes Sep 23, 2020

View reviewed changes

Address part of Tom R comments

a10a0e8

pllim added 2 commits September 23, 2020 11:45

Replace pep517 with python-build

e84d2d6

Fix minor typo

807837c

Cadair approved these changes Sep 23, 2020

View reviewed changes

saimn approved these changes Sep 27, 2020

View reviewed changes

pllim mentioned this pull request Sep 28, 2020

DOC: Update/remove command history files #10777

Closed

astrofrog approved these changes Sep 29, 2020

View reviewed changes

astrofrog merged commit 4f18f9c into astropy:master Sep 29, 2020

pllim deleted the no-setup-py branch September 29, 2020 15:13

eteq pushed a commit to eteq/astropy that referenced this pull request Oct 12, 2020

Merge pull request astropy#10756 from pllim/no-setup-py

537185c

MNT: Update doc about setup.py and py.test

eteq pushed a commit that referenced this pull request Oct 13, 2020

Merge pull request #10756 from pllim/no-setup-py

fcfcea3

MNT: Update doc about setup.py and py.test

bsipocz modified the milestones: v4.1.1, v4.2 Nov 25, 2020

bsipocz modified the milestones: v4.2, v4.1 Nov 25, 2020

		@@ -305,7 +303,7 @@ accuracy/test_icrs_fk5.py .

		# So far, so good, now check ALL tests
		$ cd ../../.. # up to the top level to check all of the tests

MNT: Update doc about setup.py and py.test #10756

MNT: Update doc about setup.py and py.test #10756

Conversation

pllim commented Sep 22, 2020 • edited Loading

Description

saimn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Sep 22, 2020

astrofrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Sep 23, 2020 • edited Loading

pllim commented Sep 23, 2020

saimn left a comment

Choose a reason for hiding this comment

astrofrog left a comment

Choose a reason for hiding this comment

bsipocz commented Nov 25, 2020

pllim commented Sep 22, 2020 •

edited

Loading

pllim Sep 23, 2020 •

edited

Loading

pllim commented Sep 23, 2020 •

edited

Loading