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

Add 'dist' build #5656

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Add 'dist' build #5656

merged 1 commit into from
Dec 14, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Dec 5, 2017

Description

This is a continuation of #5624, proposing that the package be moved under a src dir. To recap:

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.

This iteration fixes issues with coverage data. Most of the builds do use the local files via an editable install. As such, all coverage data is under /src/rest_framework instead of spread across multiple venvs. Testing the distribution is done wit a single build that doesn't collect coverage data.

As far as I can tell, the only potential downside is that git blame will be hampered a bit, given the file moves.

Changes

  • Moved package under src dir
  • Added dist build.
  • Updated support builds (lint, docs) to use python 3.6

@tomchristie
Copy link
Member

proposing that the package be moved under a src dir

On first sight I'm not in favour of us having to change a several year-long layout, in order to address tooling issues.

@tomchristie
Copy link
Member

Could we do something like ensure that the script cd's into a different directory when testing the distribution? Any other options?

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 5, 2017

TBH, I'm fine with this. Other options are possibly worst in my opinion.
Some reasons are explained here but there are a lot of other links that would tell more about.

@rpkilby
Copy link
Member Author

rpkilby commented Dec 5, 2017

Could we do something like ensure that the script cd's into a different directory when testing the distribution?

Changing directories won't have an effect, but we could add a flag to runtests that enables removing the script's directory from sys.path.

@tomchristie
Copy link
Member

Changing directories won't have an effect, but we could add a flag to runtests that enables removing the script's directory from sys.path.

I'd far prefer that, yup. If we can make a more minimal change, then we should.

@rpkilby
Copy link
Member Author

rpkilby commented Dec 5, 2017

Sounds good. I'll update the PR later.

@tomchristie
Copy link
Member

I'll update the PR later.

Okey doke. I suppose either rebase, or issue a new PR and close this.

@rpkilby rpkilby changed the title Move package into 'src' dir, add 'dist' build Add 'dist' build Dec 5, 2017
@rpkilby rpkilby force-pushed the dist-build branch 2 times, most recently from 909ba98 to e0a77ce Compare December 6, 2017 16:43
@rpkilby
Copy link
Member Author

rpkilby commented Dec 6, 2017

Okay, this should be good to go now. There was an issue in that pytest would re-add the package's root directory after it had been removed. The solution is to just import rest_framework, caching the installed version in sys.modules before the root directory is added back in.

Also, I temporarily committed 909ba98 to demonstrate the packaging failures (see: failing build).

@carltongibson carltongibson added this to the v3.7.4 milestone Dec 14, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK. Nice. Very neatly done. 👏

@carltongibson carltongibson merged commit d12005c into encode:master Dec 14, 2017
@rpkilby rpkilby deleted the dist-build branch December 14, 2017 12:12
@rpkilby rpkilby mentioned this pull request Dec 21, 2017
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
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.

4 participants