Vendoring-compatible imports

[jku]

The goal here is to make TUF compatible with the vendoring tool so that tuf sources can be vendored into pip. As a reminder vendoring will modify all import-lines in a vendored project like this patch:

- import package 
- from package import module 
- from package.module import func
+ from _vendoring import package 
+ from _vendoring.package import module 
+ from _vendoring.package.module import func

This does not work in the case of import package.module that is so common in tuf.

Notes

• tuf modules are now imported with this style from tuf import download. This works with just a few localised issues: I'm generally happy with how code calling internal modules looks
• securesystemslib modules can't be handled this way:
  • module names clash with tuf modules names
  • module names are quite generic and likely to lead to conflicts later on
• as result securesystemslib imports are now like from securesystemslib import formats as sslib_formats in most cases. I don't particularly like it but it is readable and I can't think of a better solution
• Very minor changes to how other dependencies are used
• linter now understands our imports so some fixes and workarounds have been added.
• The import lists that we're changed have been sorted so tuf imports are last
• The patch is surprisingly large, ~900 lines. Roughly 350 are needed for the client vendoring but I modified the whole code base to at least be consistent. The tests have not been touched.

[joshuagl]

Thank you for tackling this @jku!

FYI the series has multiple "imports: Fix securesystemslib.exceptions imports" patches. They aren't all modifying exceptions. Similarly with "imports: Fix securesystemslib.interface imports".

This is a draft since I'll have to do something similar in securesystemslib before I can test that it all actually works with vendoring... I am also going to go through this PR again to make sure there are no typos and to improve commit messages.

Should have paid more attention to this before reviewing commit by commit, I hope the numerous comments I've left are useful.

• as result securesystemslib inports are now like from securesystemslib import formats as sslib_formats in most cases. I don't particularly like it and am willing to consider alternatives

I'm not opposed to this, though I should point out that we can avoid many of the sslib_* imports by importing only the names we care about, you've done this in a couple of places – importing only the functions from a securesystemslib module that the tuf module is using – I assume expediency is the main reason you didn't do this throughout?

• The patch is surprisingly large, ~900 lines. Roughly 350 are needed for the client vendoring but I modified the whole code base to at least be consistent. The tests have not been touched.

Completely agree with this rationale.

[lukpueh]

Thanks a ton for taking a stab at this thankless task, and especially for the thorough separation of commits! As requested, I only skimmed the patches here and in secure-systems-lab/securesystemslib#316. Let me know when you want a full review.

Here are some thoughts regarding your notes from above.

• tuf modules are now imported with this style from tuf import download.

I'd like to add that this does not only fix the vendoring issues but also aligns with our style recommendation, even though the Google guide accepts both.

• as result securesystemslib inports are now like from securesystemslib import formats as sslib_formats in most cases. I don't particularly like it and am willing to consider alternatives

I'm fine with this approach. IMO the secondary goal (besides making vendoring work) should be to help the reader attribute function or other names to modules or packages. And sslib_interface.import_publickeys_from_file seems to serve that purpose better than interface.import_publickeys_from_file.

• The import lists that we're changed have been sorted so tuf imports are last

Excellent, this too is a style guide recommendation.

• The patch is surprisingly large, ~900 lines. Roughly 350 are needed for the client vendoring but I modified the whole code base to at least be consistent. The tests have not been touched.

👍

Do you think it's worth to add an ADR for the rationale in this PR? Or to amend our style guide? Specifically in regards to:

from package import module vs. from package import module as xyz_module vs. from package.module import name

[jku]

To you think it's worth to add an ADR for the rationale in this PR? Or to amend our style guide? Specifically in regards to:

from package import module vs. from package import module as xyz_module vs. from package.module import name

Eh, I think it maybe depends... So far I've mostly used the first (and second where confusion was more likely) but the third one makes sense for some modules. I've avoided the third option for now mostly to keep the changes to a minimum and so I only had to worry about one new local name (the module name) and not multiple as might be the case if we import individual things from modules.

Related thought (this might not be a good idea, just something I'm wondering), for some of our most common imports I've been thinking: If TUF uses e.g. schemas from tuf.formats and securesystemslibs.formats wouldn't it make sense for tuf.formats to expose the relevant items from securesystemslibs.formats? This way there would be one place (tuf.formats) that takes care of possible conflicts and other TUF code could just import tuf.formats or individual items from there (module name not really needed because now there's no confusion over where they come from). Same goes to a degree for exceptions -- where it's a bit weird that code that does not call securesystemslib code might have to import securesystemslib.exceptions to handle errors.

[lukpueh]

Related thought (this might not be a good idea, just something I'm wondering), for some of our most common imports I've been thinking: If TUF uses e.g. schemas from tuf.formats and securesystemslibs.formats wouldn't it make sense for tuf.formats to expose the relevant items from securesystemslibs.formats?

I generally think that's a good idea, but given that we want to get away from schemas (secure-systems-lab/securesystemslib#183) maybe not worth the effort?

Same goes to a degree for exceptions -- where it's a bit weird that code that does not call securesystemslib code might have to import securesystemslib.exceptions to handle errors.

I agree that it's weird. But I think the solution should be a revision of the exception taxonomy (#1131, secure-systems-lab/securesystemslib#191). AFAICS there is no big win in defining any tuf exceptions in securesystemslib.

[jku]

I've marked this ready for review.

I think this is the one question that hasn't been answered (in this issue):

I'm not opposed to this, though I should point out that we can avoid many of the sslib_* imports by importing only the names we care about, you've done this in a couple of places – importing only the functions from a securesystemslib module that the tuf module is using – I assume expediency is the main reason you didn't do this throughout?

I think the places where I did it were already using this format for some other formats from this module (so I was aiming for least disruption to existing code). Otherwise I tried to follow the guideline of importing modules, not module contents. It's true that in some instances the latter would work better: schemas are an example but modifying them is a lot of work that might not be 100% trivial and that I'd rather leave for a follow up if needed

[lukpueh]

Thanks again for doing this dull deed, @jku! Here is how I reviewed your PR:

I have not verified if this patch

• indeed makes tuf vendoring-compatible,
• is complete,
• doesn't shadow any names.

Also, I have not commented on inconsistent adoption of new module/sybmol paths in docstrings, most notably in <Exceptions>, or Raises: sections. I noticed some and briefly considered pointing them out, but I'm unsure if it is worth the effort.

What I did do is I

• went through each commit,
• read each diff line by line, and
• checked that each diff corresponds to the commit message.
• I also made sure you addressed @joshuagl's comments.

Based on that, your PR LGTM.

If that's enough, I'd say we merge, after you have rebased and fixed the conflict in tuf/api/metadata.py.

ps: I'd also like to land this before #1314 because that way it will be easier to resolve conflicts (nothing big, I checked, but still).

[lukpueh]

Nit pick: I think the patch in this file should have gone into 7841008. (Note that I mostly point this out so that you see that I actually read the diff 😄 . Feel free to ignore.)

[jku]

Changes since last push:

• rebased to develop
• fixed rebase issues in "imports: Make 'formats' imports vendoring-compatible"
• fixed rebase issues in "imports: Make 'exceptions' imports vendoring-compatible"
• Added a commit for a missed requests_fetcher import
• Added a commit to bundle imports like isort likes them

[lukpueh]

Still LGTM! :)