@include: Only include modules from the current package

[SitiSchu]

Fixes #1483

The previously used SourceFileLoader(name, filepath).load_module() adds the imported module to sys.modules. When a second module is imported with the same name, the previous module gets overwritten. In the case of #1483 this happened:

• Pkg A utils gets imported and added to the module cache with its hash
  • Module gets added to sys.modules
• Pkg B utils gets imported and added to the module cache with its hash.
  • This replaces A utils in sys.modules which causes the module cache to now have Pkg B's utils for all utils hashes
• Pkg C utils' hash is in the module cache from Pkg A, so it gets used from the cache. However, Pkg B utils replaced all utils modules so this returns Pkg B's utils.

Using importlib.util.module_from_spec avoids this by not adding imported modules to sys.modules so nothing gets overwritten even if modules with the same name get imported multiple times.

Since the @include functionality works by placing the module in the globals for exec, not having the module in sys.modules should be fine.

[JeanChristopheMorinPerso]

Thanks for the fix @SitiSchu! Do you think you could add a simple unit tests there to make sure this is well covered?

[JeanChristopheMorinPerso]

Note to TSC: CLA is correctly signed and setup. I'm not too sure why the EasyCLA bot didn't create a comment. I checked in PCC and I see the signature, so everything is good.

[SitiSchu]

@JeanChristopheMorinPerso Sure can do, what would you like tested? I guess making sure it doesn't actually end up in sys.modules and maybe ensuring all defined functions and variables are accessible?

[JeanChristopheMorinPerso]

I'd test get_module_from_file, which basically means testing that I can import from a given file and that it doesn't end up in sys.modules.

[SitiSchu]

I've added a test case for this.

To make sure it works, I ran the test without my fix and it did fail correctly.

[SitiSchu]

It looks like the test fails with Python 2.7, should I limit the test to Python 3 only or should I try and fix the issue for py2 too(despite it being deprecated for years now)?

[JeanChristopheMorinPerso]

We need to support both python 2 and 3. So if we fix python 3, we also need a fix for py 2.

[SitiSchu]

Locally the copyright fix added a empty line to the utils file which made git complain about the extra whitespace. So I added a comment. Now it doesn't complain locally anymore but somehow the CI still fails.

[JeanChristopheMorinPerso]

Don't worry about this check for now. We can take care of that.

Thanks for adding a test!

[JeanChristopheMorinPerso]

There you go @SitiSchu. I fixed the copyright issue. I remove the dummy python module and I'm now creating it on the fly in the tests. I wasn't able to find why the change_copyright isn't doing what it's supposed to.

[JeanChristopheMorinPerso]

LGTM, thanks a lot for this!

[JeanChristopheMorinPerso]

Thanks you @SitiSchu !