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

Fix packaging #5624

Merged
merged 2 commits into from Nov 27, 2017
Merged

Fix packaging #5624

merged 2 commits into from Nov 27, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 25, 2017

I've punted on moving the package under a src directory, and just made the minimum necessary changes. Also, fixes #5615.


I don't know if we want to accept this as is, given that I moved the package contents into a src directory. If we're willing to accept that change, the move is for a good reason. tox does create and install the package distribution into the test environment, however this conflicts with the rest_framework package that's located on the current path. Because the distribution and local files conflict, you cannot effectively test the distribution's packaged files. eg, the distribution may not be correctly configured (missing templates/static files), but the files on the local path do have them, so the tests pass erroneously.

The solution is to move the package off of the path (usually into a src directory), so that the local files are not importable.

The alternative would be to not move the package into src and accept that we may accidentally leave out files from time to time. This is probably fine, given that the manifest won't change very often.

@rpkilby rpkilby force-pushed the fix-packaging branch 2 times, most recently from c97e9c6 to bccd324 Compare Nov 25, 2017
@rpkilby rpkilby changed the title [wip] Fix packaging Fix packaging? Nov 25, 2017
Copy link
Contributor

@auvipy auvipy left a comment

what benefit will be added by moving under src/ ?

will that create any backward incompatibility?

@rpkilby
Copy link
Member Author

rpkilby commented Nov 25, 2017

@auvipy. The core issue is that tox, while it does create and install a distribution for DRF, the files in the distribution conflict with those found on the local path. This is usually fine, however, it prevents you from finding packaging issues. eg, your distribution may be borked (missing templates and static files), but testing passes erroneously because it can find these files under the local path. By moving the package on the local path into a src directory, they're no longer directly importable (and as a side effect, the templates/static files are no longer found). Thus, tox is just testing the distribution, so you have better assurance that your distribution is correct.

@rpkilby
Copy link
Member Author

rpkilby commented Nov 25, 2017

As to backward incompatibility, there shouldn't be any issues. At the end of the day, the distribution still contains a single rest_framework package. The only potential issues would be files missing from the distribution due to misconfiguration. Although, that's what this PR aims to fix by moving the package into a src directory.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 25, 2017

I don't think there'll be issue since it's just about the repository layout.
I'm ok with the overall idea.

@rpkilby rpkilby force-pushed the fix-packaging branch 2 times, most recently from bccd324 to 4636447 Compare Nov 26, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Nov 26, 2017

I'd rather punt on moving the package under a src directory for now. It creates issues with the coverage report, as the files are no longer under rest_framework, but under site-packages in the test env.


Notes to future myself about moving the package under src:

  • tox testenvs should use the usedevelop option, as opposed to installing a competing sdist. This keeps the coverage data from breaking.
  • to test the packaging, specifically add a dist testenv. It's not necessary to test the dist against every testenv in the matrix.

@carltongibson carltongibson added this to the v3.7.4 milestone Nov 27, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks good. (And fixes #5615! 🙂)

@carltongibson carltongibson merged commit abef84f into encode:master Nov 27, 2017
1 check passed
@rpkilby rpkilby deleted the fix-packaging branch Nov 27, 2017
@@ -6,7 +6,7 @@
import sys
from io import open

from setuptools import setup
from setuptools import setup, find_packages
Copy link
Member

@tomchristie tomchristie Nov 27, 2017

Choose a reason for hiding this comment

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

Nice work!

@rpkilby rpkilby changed the title Fix packaging? Fix packaging Dec 5, 2017
@rpkilby rpkilby mentioned this pull request Dec 5, 2017
@ryanhiebert
Copy link
Contributor

ryanhiebert commented Dec 21, 2017

I'd rather punt on moving the package under a src directory for now. It creates issues with the coverage report, as the files are no longer under rest_framework, but under site-packages in the test env.

This is something I had to overcome with tox-travis as well. It manages it by running coverage in parallel mode, then combining paths to be the files in the src directory. It can be a little confusing to figure out where the lines should be on using combine, etc, but it does seem to work get the job done in my experience.

I'm not sure how much tox-travis has informed your knowledge here, but I do think that I got it worked out OK there. You can check out what I did in the tox-travis tox.ini file for the coverage configuration.

@rpkilby
Copy link
Member Author

rpkilby commented Dec 21, 2017

Thanks @ryanhiebert. I was able to get coverage to work properly in #5656, but moving the package into a src directory was decided against. Instead there's a single build that tests the packaging w/o coverage.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Packaging should use manifest

* Add schema.js template to MANIFEST
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants