Move the cryptography package into a src/ subdirectory #1468

dstufft · 2014-11-08T00:29:01Z

Due to differences in how py.test determines which module to import the test suite actually runs against the cryptography which is in the current directory instead of the cryptography which is installed. The problem essentially boils down to when there is a tests/init.py then py.test adds the current directory to the front of the sys.path, causing it to take precedence over the installed location.

This means that running the tests relies on the implicit compile that CFFI does instead of testing against what people will actually be runnning, which is the module compiled by setup.py.

This should also cause the tests to run a little bit faster as something like tox -e py34 currently compiles everything 3 times instead of only once like it should. This PR will drop this down to 2 times.

dstufft · 2014-11-08T00:31:28Z

To be clearer, this essentially "fixes" the problem by making it so that there is no top level cryptography importable in the current directory by moving it one level deeper.

jenkins-cryptography · 2014-11-08T00:46:20Z

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2517/

jenkins-cryptography · 2014-11-08T01:01:13Z

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2518/

alex · 2014-11-08T01:05:14Z

Eep. Is there really no other way to do this? It feels like a pretty obscure solution.

dstufft · 2014-11-08T01:07:41Z

When I did this for dstufft/http11 I couldn't figure out a better way. It doesn't feel very clean I agree :(

alex · 2014-11-08T01:11:03Z

Why do we need this if we're removing implicit compile? Shouldn't that get rid of the stuff in the top level directory?

dstufft · 2014-11-08T01:13:35Z

I explained it here: #1467 (comment) but essentially the way pytest works it means import cryptography isn't using the version of cryptography installed into the site-packages directory (which is where the compiled modules are) but it's using the version at the top level here. It only works right now because cffi implicitly compiles. If we disable the implicit compile without this PR then we just throw errors attempting to collect the tests because it can't locate the compiled module and can't implicitly compile another one.

dstufft · 2014-11-08T01:16:08Z

Important to note, that the way pytest works also means that currently we are not testing against an installed copy of cryptography, but against a checkout. This means that if our setup.py doesn't correctly install cryptography (but it "successfully" completes the installation, e.g. has a zero exit code) the test suite will still run successfully. However with this change we'll actually be testing against an installed copy of cryptography so we'll also be testing that everything works when installed.

alex · 2014-11-08T01:21:40Z

Sorry, if installation succeeds, why can't it locate the compiled module?
We did compile it.

On Fri Nov 07 2014 at 10:16:08 PM Donald Stufft notifications@github.com
wrote:

Important to note, that the way pytest works also means that currently we
are not testing against an installed copy of cryptography, but against
a checkout. This means that if our setup.py doesn't correctly install
cryptography (but it "successfully" completes the installation, e.g. has a
zero exit code) the test suite will still run successfully. However with
this change we'll actually be testing against an installed copy of
cryptography so we'll also be testing that everything works when installed.

—
Reply to this email directly or view it on GitHub
#1468 (comment).

dstufft · 2014-11-08T01:24:03Z

Because installing cryptography within tox puts the installed copy of cryptography, along with the compiled module in .tox/pyXY/lib/pythonX.Y/site-packages/. However when tests/__init__.py exists pytest puts . as the first item in sys.path. This means that the test suite will be importing cryptography from . instead of .tox/pyXY/lib/pythonX.Y/site-packages/.

dstufft · 2014-11-08T01:26:13Z

IOW we have two copies of cryptography on sys.path the one in . which is not installed and the one in .tox/pyXY/lib/pythonX.Y/site-packages which is installed. I believe pytest adds . to sys.path so that it can import the test modules (and when it imports those test modules, it also imports cryptography.

alex · 2014-11-08T01:31:10Z

What happens if you delete the tests/__init__.py?

On Fri Nov 07 2014 at 10:26:13 PM Donald Stufft notifications@github.com
wrote:

IOW we have two copies of cryptography on sys.path the one in . which is
not installed and the one in .tox/pyXY/lib/pythonX.Y/site-packages
which is installed. I believe pytest adds . to sys.path so that it can
import the test modules (and when it imports those test modules, it also
imports cryptography.

—
Reply to this email directly or view it on GitHub
#1468 (comment).

dstufft · 2014-11-08T01:35:25Z

I believe (I'd need to test it) that we'll correctly use the installed copy of cryptography without needing to shuffle it into a src/ directory to make it unimportable when . is added to sys.path (because pytest will use a different method instead of import to load the test modules). However I don't think we'll be able to import things like tests.utils then and we won't be able to reuse test module names (like right now we have 2x test_utils.py, 2x test_openssl.py, 2x test_commoncrypto.py).

I remember trying that before with http11 (and it was my original solution) until I started running into issues with it. I think the above were the issues I ran into. I can try it again to see if my memory is correct but it'll be a little bit as I'm about to step out for 20 minutes or so.

alex · 2014-11-08T01:38:06Z

Ok, if you could try that out it'd be great. If not, I wonder if we should
bring this up with the py.test folks, it seems like a pretty unfortunate
intersection of issues.

On Fri Nov 07 2014 at 10:35:26 PM Donald Stufft notifications@github.com
wrote:

I believe (I'd need to test it) that we'll correctly use the installed
copy of cryptography without needing to shuffle it into a src/ directory
to make it unimportable when . is added to sys.path (because pytest will
use a different method instead of import to load the test modules). However
I don't think we'll be able to import things like tests.utils then and we
won't be able to reuse test module names (like right now we have 2x
test_utils.py, 2x test_openssl.py, 2x test_commoncrypto.py).

I remember trying that before with http11 (and it was my original
solution) until I started running into issues with it. I think the
above were the issues I ran into. I can try it again to see if my memory is
correct but it'll be a little bit as I'm about to step out for 20 minutes
or so.

—
Reply to this email directly or view it on GitHub
#1468 (comment).

alex · 2014-11-08T01:38:41Z

FWIW, we never do "from tests", we always do "from ." for imports there, so
it's not important the name "tests" be on the path.

On Fri Nov 07 2014 at 10:35:26 PM Donald Stufft notifications@github.com
wrote:

I believe (I'd need to test it) that we'll correctly use the installed
copy of cryptography without needing to shuffle it into a src/ directory
to make it unimportable when . is added to sys.path (because pytest will
use a different method instead of import to load the test modules). However
I don't think we'll be able to import things like tests.utils then and we
won't be able to reuse test module names (like right now we have 2x
test_utils.py, 2x test_openssl.py, 2x test_commoncrypto.py).

I remember trying that before with http11 (and it was my original
solution) until I started running into issues with it. I think the
above were the issues I ran into. I can try it again to see if my memory is
correct but it'll be a little bit as I'm about to step out for 20 minutes
or so.

—
Reply to this email directly or view it on GitHub
#1468 (comment).

jenkins-cryptography · 2014-11-08T01:46:24Z

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2519/

dstufft · 2014-11-08T02:03:05Z

Yea it is, If my understanding is correct I'm not sure if it's reasonably avoidable. I think it adds . to sys.path so that it can use the Python import system to import the test modules (which is what enables the relative imports and the like). However importing the tests modules also imports cryptography so I don't think it's possible without moving the cryptography directory to enable using the import mechanisms to discover tests without also importing cryptography from the top dir.

I'm back now, going to be testing that.

dstufft · 2014-11-08T02:08:08Z

Yea so I'm correct you can't do relative imports without the __init__.py. Switching to master and deleting tests/**/__init__.py results in:

tox -e py34 tests/hazmat/primitives/test_arc4.py                                                                                                                                                          44s  ⦕cryptography⦖
GLOB sdist-make: /Users/dstufft/projects/cryptography/setup.py
py34 inst-nodeps: /Users/dstufft/projects/cryptography/.tox/dist/cryptography-0.7.dev1.zip
py34 runtests: PYTHONHASHSEED='2008848676'
py34 runtests: commands[0] | coverage run -m pytest --capture=no --strict tests/hazmat/primitives/test_arc4.py
Traceback (most recent call last):
  File "/Users/dstufft/projects/cryptography/.tox/py34/lib/python3.4/site-packages/_pytest/config.py", line 513, in getconftestmodules
    return self._path2confmods[path]
KeyError: local('/Users/dstufft/projects/cryptography/tests/hazmat/primitives/test_arc4.py')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/Users/dstufft/projects/cryptography/.tox/py34/lib/python3.4/site-packages/_pytest/config.py", line 537, in importconftest
    return self._conftestpath2mod[conftestpath]
KeyError: local('/Users/dstufft/projects/cryptography/tests/conftest.py')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/Users/dstufft/projects/cryptography/.tox/py34/lib/python3.4/site-packages/_pytest/config.py", line 543, in importconftest
    mod = conftestpath.pyimport()
  File "/Users/dstufft/projects/cryptography/.tox/py34/lib/python3.4/site-packages/py/_path/local.py", line 641, in pyimport
    __import__(modname)
  File "/Users/dstufft/projects/cryptography/tests/conftest.py", line 20, in <module>
    from .utils import check_backend_support, select_backends, skip_if_empty
SystemError: Parent module '' not loaded, cannot perform relative import
ERROR: could not load /Users/dstufft/projects/cryptography/tests/conftest.py

alex · 2014-11-08T03:57:23Z

This now needs a rebase.

I'd like to hear what other folks think about this. I'm not wild about it, but there don't really seem to be any other alternatives :-/

dstufft · 2014-11-08T04:07:09Z

Rebased.

With this there should now be a single compile step taken no matter what situation we're in. The implicit compiles should be completely gone from any installed situation. The implicit compile is still enabled in the code though, so bugs could still cause one to happen.

jenkins-cryptography · 2014-11-08T04:21:36Z

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2523/

reaperhulk · 2014-11-08T07:16:54Z

In the past we've resisted putting the tests inside the cryptography package itself (although we've had some users, especially in the distro packaging world, request this). Would moving tests to cryptography.tests resolve this issue?

alex · 2014-11-08T13:21:35Z

Someone would have to check, but I think yes, it owuld.

dstufft · 2014-11-08T18:59:53Z

It might, it depends on if if py.test will discover the tests inside the installed directory or inside the current directory. Let me try it.

dstufft · 2014-11-08T19:09:47Z

No difference, if I move the tests to cryptography.tests they work, just as they work now, however they still rely on the implicit compilation (tested by disabling the implicit compilation and running the tests). This would probably work if pytest didn't do the "look in current directory to attempt to find tests" things and worked more like unitest or so where you have to define an import statement.

dstufft · 2014-11-08T19:12:13Z

One other possible solution is to disallow relative imports in the tests, which would mean that all files have to be self contained and all of the utility stuff has to live inside the cryptography project.

dstufft · 2014-11-08T20:40:53Z

The setup.py build_ext --inplace solution also means we'll end up compiling twice during test runs still, once to install it into the site-packages folder, and once to install it into ..

alex · 2014-11-08T20:45:16Z

Do you think testing against the installed copy is the right way to do
stuff? For projects that don't use tox, I suppose almost all of them are
just testing against the source version.

On Sat Nov 08 2014 at 5:40:58 PM Donald Stufft notifications@github.com
wrote:

The setup.py build_ext --inplace solution also means we'll end up
compiling twice during test runs still, once to install it into the
site-packages folder, and once to install it into ..

—
Reply to this email directly or view it on GitHub
#1468 (comment).

dstufft · 2014-11-08T20:53:57Z

I think testing against an installed copy is going to give you results which are most likely to reflect the real world. In particular it's the best way to find bugs in your packaging where your installation commands succeed (defined as exiting with a 0 exit code) but leave the installed project in a broken state (missing file or something similar). The most common case for this is missing a file out of MANIFEST.in, packages, or package_data. In cryptography's case we'd probably fail installation if the thing we left out was something required for installation (like a .c file) but we would not fail for something that was only used at runtime (nothing in cryptography itself currently matches this, the vector files would count here though if they were part of cryptography).

We automate the packages and package_data information via setuptools, so we could say that we trust setuptools to properly handle those things and then we could use something like check-manifest to try and detect when something has been accidentally left out of a MANIFEST.in.

With the above, we lower the chance of something going undetected, though it's still technically possible through bugs or just edge cases. Whether we care is a trade off question.

dstufft · 2014-11-08T20:57:09Z

#1470 has the setup.py build_ext --inplace solution.

alex · 2014-11-12T00:56:45Z

Before I can render a final opinion on this, I'd like to think about how the overall tree would/should look. Specifically, where should we put vectors and maybe other in-tree backends.

dstufft · 2014-11-12T01:32:24Z

Probably anything that is going to be installable should be inside the src/ dir. I had left the vectors package out because it's in this wierd limbo where it is it's own directory that has it's own setup.py and the like.

alex · 2014-11-12T01:47:38Z

Right, what's teh right way to handle the vectors setup.py, and license
(which are the same, we coudl delete the dupes)

On Tue Nov 11 2014 at 10:32:25 PM Donald Stufft notifications@github.com
wrote:

Probably anything that is going to be installable should be inside the
src/ dir. I had left the vectors package out because it's in this wierd
limbo where it is it's own directory that has it's own setup.py and the
like.

—
Reply to this email directly or view it on GitHub
#1468 (comment).

dstufft · 2014-11-12T01:52:22Z

I don't really think there's a better way then how we're handling it unless we wanted to move everything down a level and have cryptography/setup.py, and cryptography_vectors/setup.py at the top level of the repository. The tooling doesn't really grok multiple python packages in one repo very well so it's kind of a wierd edge case. pip does recently have support for passing a sub directory to locate the setup.py into it.

I think I said it at the time, but I'm personally (still?) of the opinion that vecotrs should really live in their own repository.

public · 2014-11-12T10:40:07Z

I am OK with this in principle. I think vectors should live within src too for now.

dstufft · 2014-11-12T12:28:02Z

So you think it should be src/vectors/{cryptography_vectors,setup.py,...}?

Due to differences in how py.test determines which module to ``import`` the test suite actually runs against the cryptography which is in the *current* directory instead of the cryptography which is installed. The problem essentially boils down to when there is a tests/__init__.py then py.test adds the current directory to the front of the sys.path, causing it to take precedence over the installed location. This means that running the tests relies on the implicit compile that CFFI does instead of testing against what people will actually be runnning, which is the module compiled by setup.py.

When using coverage.py with a project installed into site-packages instead of in the current directory you end up with paths like .tox/py34/lib/python3.4/site-packages/cryptography/__init__.py which is less than ideal (and may cause issues when aggregating coverage over multiple versions of Python). Switching coverage.py into parallel-mode will have it write a .coverage.* file instead of a .coverage file, which can then be "combined" into a .coverage file using coverage combine. When coverage.py does the combine step it will collapse the .tox/*/lib/python*/site-packages/cryptography paths into src/cryptography.

dstufft · 2014-11-13T13:21:34Z

I rebased this, and included a fix for coverage so instead of it reporting paths like .tox/py34/python3.4/site-packages/cryptography/__init__.py it will report paths like src/cryptography/__init__.py even though it's actually testing the coverage of .tox/py34/python3.4/site-packages/cryptography/__init__.py.

jenkins-cryptography · 2014-11-13T13:35:44Z

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2530/

dstufft · 2014-11-13T14:04:35Z

One thing to note: If this gets merged and people have problems where coverage information doesn't show up, they should make sure they don't have an empty cryptography folder still sitting around in their check out. I've had that happen once or twice when I was rebasing. It only had .pyc files in it but it confused coverage.py.

Ayrx · 2014-11-14T14:25:31Z

Just to be clear on the implications of this, my WIP scrypt backend will live in src/cryptography_libscrypt/foo?

dstufft · 2014-11-14T14:28:26Z

Does it use the top level setup.py?

Ayrx · 2014-11-14T14:31:39Z

No, it's a self-contained package ala vectors.

dstufft · 2014-11-14T14:35:15Z

Oh I see, it's another thing like vectors, no that wouldn't move into src/ unless we also move the vectors folder there.

Unrelated, if we're going to keep adding more separate packages to this repository instead of putting each package in it's own repository, then we should do something like:

.
├── cryptography
│   ├── setup.py
│   └── src
│       └── cryptography
├── cryptography_scrypt
│   ├── cryptography_scrypt
│   └── setup.py
└── cryptography_vectors
    ├── cryptography_vectors
    └── setup.py

I also am pretty down on cramming all of these packages into the same repository. It sort of made sense with the vectors because the only reason vectors was a separate package was to make the tarball download smaller, however in general I think it's a pretty subpar way to organize things.

dstufft · 2014-11-14T14:51:24Z

Ayrx · 2014-11-14T14:53:20Z

👍 LGTM

Move the cryptography package into a src/ subdirectory

dstufft · 2014-11-14T18:02:12Z

dstufft mentioned this pull request Nov 8, 2014

Refactor to avoid the implicit compile when executing setup.py #1467

Merged

dstufft force-pushed the move-to-src branch from b7ccc56 to 16bbb75 Compare November 8, 2014 00:45

dstufft mentioned this pull request Nov 8, 2014

Unable to cross compile due to cffi (from pyopenssl) #1325

Closed

dstufft force-pushed the move-to-src branch from fe3eab1 to 4386a18 Compare November 8, 2014 04:03

dstufft added 2 commits November 13, 2014 07:56

dstufft force-pushed the move-to-src branch from 4386a18 to 62143e4 Compare November 13, 2014 13:20

reaperhulk added this to the Seventh Release milestone Nov 14, 2014

reaperhulk added a commit that referenced this pull request Nov 14, 2014

Merge pull request #1468 from dstufft/move-to-src

af3d95f

Move the cryptography package into a src/ subdirectory

reaperhulk merged commit af3d95f into pyca:master Nov 14, 2014

dstufft deleted the move-to-src branch November 14, 2014 18:02

This was referenced Aug 20, 2018

Fix codecov coverage by collapsing paths in tox urllib3/urllib3#1428

Merged

Compute coverage only for the sync urllib3 python-trio/hip#50

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2020

Move the cryptography package into a src/ subdirectory #1468

Move the cryptography package into a src/ subdirectory #1468

Conversation

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

jenkins-cryptography commented Nov 8, 2014

jenkins-cryptography commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

alex commented Nov 8, 2014

jenkins-cryptography commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

jenkins-cryptography commented Nov 8, 2014

reaperhulk commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 8, 2014

dstufft commented Nov 8, 2014

dstufft commented Nov 8, 2014

alex commented Nov 12, 2014

dstufft commented Nov 12, 2014

alex commented Nov 12, 2014

dstufft commented Nov 12, 2014

public commented Nov 12, 2014

dstufft commented Nov 12, 2014

dstufft commented Nov 13, 2014

jenkins-cryptography commented Nov 13, 2014

dstufft commented Nov 13, 2014

Ayrx commented Nov 14, 2014

dstufft commented Nov 14, 2014

Ayrx commented Nov 14, 2014

dstufft commented Nov 14, 2014

dstufft commented Nov 14, 2014

Ayrx commented Nov 14, 2014

dstufft commented Nov 14, 2014