Vendoring-compatible imports #316

jku · 2021-01-14T14:19:06Z

This is a sister PR for theupdateframework/python-tuf#1261: it modifies all imports in securesystemlibs to be compatible with vendoring tool (see TUF PR for explanation).

Notes

Multiple solutions were used, but usually the module was directly imported into scope (so instead of referring to securesystemslib.settings code now refers to settings). In some cases this individual items from the module are imported instead
I've tried very hard to ensure no namespace conflicts happen by grepping for every new top level name
This is still a draft: I have yet to test that vendoring actually works with this
The change is surprisingly large, >500 lines

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

Make hash module imports compatible with the vendoring tool. This is trickier than the other cases: * "hash" is a builtin function, overloading that with module name seems bad * the resulting code "hash.digest(algorithm)" reads like hash is an object... Not ideal Luckily there are few functions in the module: Import the functions directly to avoid importing the module.

coveralls · 2021-01-14T14:21:44Z

Coverage increased (+0.003%) to 98.535% when pulling f0d8fbb on jku:vendoring-compatible-imports into 0a8bde5 on secure-systems-lab:master.

coveralls · 2021-01-14T14:21:45Z

Coverage increased (+0.004%) to 98.535% when pulling 25f112b on jku:vendoring-compatible-imports into dff4425 on secure-systems-lab:master.

jku · 2021-01-21T10:57:18Z

This is ready for review: I've tested this version with pip (it now works without patching securesystemslib) and I've read through the PR again... and decided not to modify it further (without review comments anyway).

As a whole PR there is some inconsistency (that I consciously left there but could change):

I think it would usually make sense to import individual exceptions from securesystemslib.exceptions but doing that may make the PR more likely to break things: I'd rather do that in smaller PRs where possible mistakes are not lost in a sea of changes. The practical fear I have is that exceptions are sometimes not tested, so typos and name clashes could slip by without test suite noticing -- maybe linting takes care of all these cases though?
however, I have imported smaller number of individual exceptions from other sources (from cryptography or securesystemslib.gpg) where it made sense -- this leads to some exceptions being namespaced and some not
importing individual schemas from formats seems likewise a reasonable choice: I did not do that based on Lukas comment in the TUF bug but either way works for me

All of the changes are essentially modifying the import style, the only real changes are

resolved a cyclical import in gpg modules
renamed process variables to gpg_process

joshuagl

I don't think we need to address it in this PR, but I just noticed that the style guide says that we should not import names from a module:

Use import statements for packages and modules only, not for individual classes or functions.

We will want to think about a follow-on PR that addresses the issues arising from this style guide requirement, or documenting why we are not adhering to this aspect of the style guide. Exceptions, formats, and hash are obvious places where this is an issue hash. Perhaps we should file an issue about this?

I noticed that several comments are still using fully-qualified names, especially noticeable where we have comments that state which Exceptions a format module function will throw. I do not thing this is a blocker for this PR, but we should figure out how to handle this. I think it is probably bad practice in general to state in the code comments which exceptions some code might raise, it feels like it would be an unnecessary maintenance burden.

securesystemslib/keys.py

securesystemslib/interface.py

securesystemslib/hash.py

securesystemslib/interface.py

securesystemslib/util.py

securesystemslib/gpg/rsa.py

securesystemslib/gpg/functions.py

securesystemslib/gpg/util.py

jku · 2021-01-25T13:20:04Z

I don't think we need to address it in this PR, but I just noticed that the style guide says that we should not import names from a module:

Use import statements for packages and modules only, not for individual classes or functions.

We will want to think about a follow-on PR that addresses the issues arising from this style guide requirement, or documenting why we are not adhering to this aspect of the style guide. Exceptions, formats, and hash are obvious places where this is an issue. Perhaps we should file an issue about this?

I admit I had either forgotten or misunderstood the google style guide in this. I'm not totally against doing what the style guide says (it would make the PR more consistent and having less exceptions to the style guide is better) but it does feel like a hard requirement -- not having to namespace uniquely named things that are used over and over in the importing module does improve readability. Admittedly there are cases where names aren't quite unique enough and conflicts are more likely.

The other case where importing a module seems counterproductive is when there's only one or two things in the module:

from tuf.client import updater # import the module, not the class
updater = updater.Updater()    # that's not just repetitive, it's also a name clash

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

Import only the FilesystemBackend class when needed: this is compatible with the vendoring tool.

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

The aim is to avoid potential conflict with 'process' module.

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

Rename the import to make it clear this is the python module, use an import line that is compatible with vendoring tool.

Use gpg_util as local name to avoid potential conflicts with the other util module.

Importing individual errors avoids conflict with the other exceptions module in securesystemslib.

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

Importing the two functions individually seems reasonable here.

The results of the import is same, but new style is compatible with vendoring tool.

Modify the way nacl and cryptography imports are made to make them compatible with vendoring tool.

There is a circular dependency in gpg.constants -> gpg.rsa -> gpg.util -> gpg.constants This is not a problem on python3 or python2 when the imports are done with "import securesystemslib.<mod>" style. However with the "from securesystemslib import <mod>" style python2 decides this is a ImportError. Remove the circular dependency by moving the module variables from constants to another file.

jku · 2021-02-11T09:05:11Z

Applied all suggestions and rebased as none of the changes were functional, just style.

Lukas, please take a look: The resolved discussion is trivial but do check the last comment for the style guide discussion.

The latest test failure is a little baffling. I think it's unrelated but... have you seen this before?

  File "/home/travis/build/secure-systems-lab/securesystemslib/tests/test_process.py", line 121, in test_run_duplicate_streams_timeout
265    securesystemslib.process.run_duplicate_streams("python --version",
266AssertionError: TimeoutExpired not raised

lukpueh · 2021-02-11T09:42:47Z

Lukas, please take a look: The resolved discussion is trivial but do check the last comment for the style guide discussion.

Thanks, will do!

The latest test failure is a little baffling. I think it's unrelated but... have you seen this before?

No, I have not. And as coincidence would have it, @SolidifiedRay just notified me about a different and very likely also unrelated issue in the same process module.

Let me try if this issue here "goes away" if I restart the build, so that we can merge this PR and troubleshoot the process module separately.

lukpueh · 2021-02-11T10:43:15Z

Let me try if this issue here "goes away" if I restart the build ...

Looks like it did. But we should still find the cause.

joshuagl · 2021-02-16T15:47:55Z

This PR looks good to me. I believe the places we break the style guide are pragmatic choices to minimise the number of changes and/or the potential for problematic changes (for example, reducing the number of new symbols in a scope).

lukpueh

Thanks for tackling this tedious task, @jku! LGTM. All the decisions you made make sense in their context, even if you deviate from the style guide.

FYI, I also ran pylint with import-related checks only...

pylint --disable all \
       --enable $(pylint --list-msgs | grep import | grep -Eo "[A-Z][0-9]{4}" | tr "\n" ",") \
       securesystemslib

... and didn't notice anything suspicious.

lukpueh · 2021-02-16T16:45:34Z

securesystemslib/formats.py

@@ -79,7 +79,7 @@
 import six

 from securesystemslib import exceptions
-import securesystemslib.schema as SCHEMA
+from securesystemslib import schema as SCHEMA


I guess you kept as SCHEMA to make the commit less invasive? This might be something to note in the commit message.

Let me know if you want to reword the commit message. Otherwise, I'll just go ahead and merge. :)

Yes exactly. And I think I can live with that message if you can :)

Jussi Kukkonen added 3 commits January 14, 2021 12:08

imports: Make exceptions-imports compatible with vendoring

ac99f5d

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make formats-imports compatible with vendoring

fe63e10

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

lukpueh mentioned this pull request Jan 15, 2021

Vendoring-compatible imports theupdateframework/python-tuf#1261

Merged

jku marked this pull request as ready for review January 21, 2021 10:57

jku requested review from lukpueh and joshuagl January 21, 2021 10:57

joshuagl reviewed Jan 25, 2021

View reviewed changes

Jussi Kukkonen added 15 commits February 11, 2021 10:46

imports: Make "*keys"-imports compatible with vendoring

787f965

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make settings-imports compatible with vendoring

dfe9039

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make storage-imports compatible with vendoring

dea2b2a

Import only the FilesystemBackend class when needed: this is compatible with the vendoring tool.

imports: Make util-imports compatible with vendoring

0f401fb

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make schema-import compatible with vendoring

af6ae6a

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

gpg: Rename 'process' variables to 'gpg_process'

b4a069f

The aim is to avoid potential conflict with 'process' module.

imports: Make process-imports compatible with vendoring

e0f5a35

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make ed25519-import compatible with vendoring

f64ac96

Rename the import to make it clear this is the python module, use an import line that is compatible with vendoring tool.

imports: Make gpg.util-imports compatible with vendoring

3e2efa5

Use gpg_util as local name to avoid potential conflicts with the other util module.

imports: Make gpg.exceptions-imports compatible with vendoring

fdfd258

Importing individual errors avoids conflict with the other exceptions module in securesystemslib.

imports: Make gpg.constants-imports compatible with vendoring

81d60f3

Use "from securesystemslib import <mod>" instead of "import securesystemslib.<mod>" to make securesystemslib compatible with the vendoring tool.

imports: Make gpg.common-import compatible with vendoring

5150c75

Importing the two functions individually seems reasonable here.

imports: tweak imports in gpg.constants

719d315

The results of the import is same, but new style is compatible with vendoring tool.

imports: Fix external imports (crypto, nacl)

f0fbeb0

Modify the way nacl and cryptography imports are made to make them compatible with vendoring tool.

jku force-pushed the vendoring-compatible-imports branch from ae1f611 to 25f112b Compare February 11, 2021 08:51

lukpueh mentioned this pull request Feb 11, 2021

GitHub Actions: Enable windows builds #326

Open

lukpueh approved these changes Feb 16, 2021

View reviewed changes

lukpueh merged commit 3bc866c into secure-systems-lab:master Feb 17, 2021

SolidifiedRay mentioned this pull request Feb 22, 2021

in-toto Github Actions: different builds for windows fail with the same errors #337

Closed

lukpueh mentioned this pull request Feb 26, 2021

Prepare 0.20.0 release #342

Merged

jku mentioned this pull request Mar 4, 2021

import-style prevents vendoring #281

Closed

dependabot bot mentioned this pull request Mar 9, 2021

Bump securesystemslib[crypto,pynacl] from 0.19.0 to 0.20.0 CrossStream/tuf#1

Closed

Vendoring-compatible imports #316

Vendoring-compatible imports #316

Uh oh!

Conversation

jku commented Jan 14, 2021

Uh oh!

coveralls commented Jan 14, 2021

Uh oh!

coveralls commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Jan 21, 2021

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Jan 25, 2021

Uh oh!

jku commented Feb 11, 2021

Uh oh!

lukpueh commented Feb 11, 2021

Uh oh!

lukpueh commented Feb 11, 2021

Uh oh!

joshuagl commented Feb 16, 2021

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

lukpueh Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

lukpueh Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

jku Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jan 14, 2021 •

edited

Loading