Repository: ipython/ipython
Pull request: Don't add ipython unconditionally to sys.path
Load ext will already do it with a context manager.

This should remove false positive when import modules, whithout breaking
functionalities.

Maybe closes #13294 (making this part of 8.0 release make sens).

Problem statement:
Problems due to adding IPython profile dir (~/.ipython) to sys.path
When starting IPython, the profile directory gets added to sys.path. This means any `import` statement will also look for modules in that folder and in some cases allow you to import packages that aren't installed.

For instance, after using the Cython magic, I can import cython even when cython is not installed in my environment.

This causes issues when using packages/modules that rely on ImportError exceptions to check for optional features, for instance. We have a package that uses `cython.compiled` to check if one is using a plain-code or compiled version of a package. Due to IPython's behaviour regarding the profile directory, we cannot rely on the ImportError anymore - because `import cython` will just work.

I guess IPython uses the addition to sys.path to discover extensions and such. But giving the interference with module discovery, I think different ways for this should be found.

Hints from issue discussion:
Yes, this machinery is quite old, it pre-dates pip, so we should likely find other ways to do that. I think there was some recent changes in this area, maybe the master branch behaves better now.
Thanks. I will give the master branch a try soon and post the outcomes here.
The path `C:\Users\<USERNAME>\.ipython` is still being added to `sys.path` with the master branch.

The responsible code for this is this:
https://github.com/ipython/ipython/blob/9c54f3ee28612e334f6f4b609cfc2950b1de62d2/IPython/core/application.py#L289-L311

I tried removing the "sys.path.append" statement, and run IPython (and Jupyter Notebook as well), no issues in both. Also looking through the Jupyter code, I don't have the idea that it relies on having this folder in the `sys.path`. But I did not test thoroughly.
I'll have to dig arround to find out why, 
and we likely want to use context manager where relevant: https://github.com/ipython/ipython/blob/9c54f3ee28612e334f6f4b609cfc2950b1de62d2/IPython/utils/syspathcontext.py

Commit message: Don't add ipython unconditionally to sys.path

Load ext will already do it with a context manager.

This should remove false positive when import modules, whithout breaking
functionalities.

Maybe closes #13294 (making this part of 8.0 release make sens).

Patch to review:
diff --git a/IPython/core/application.py b/IPython/core/application.py
index 85c91274d4e..3a1797ab090 100644
--- a/IPython/core/application.py
+++ b/IPython/core/application.py
@@ -185,6 +185,15 @@ def _profile_changed(self, change):
                 get_ipython_package_dir(), u'config', u'profile', change['new']
         )
 
+    add_ipython_dir_to_sys_path = Bool(False,
+        """Should the IPython profile directory be added to sys path ?
+
+        This option was non-existing before IPython 8.0, and ipython_dir was added to
+        sys path to allow import of extensions present there. This was historical
+        baggage from when pip did not exist. This now default to false,
+        but can bet to true for legacy reasons
+        """).tag(config=True)
+
     ipython_dir = Unicode(
         help="""
         The name of the IPython directory. This directory is used for logging
@@ -294,21 +303,22 @@ def _ipython_dir_changed(self, change):
             str_old = os.path.abspath(old)
             if str_old in sys.path:
                 sys.path.remove(str_old)
-        str_path = os.path.abspath(new)
-        sys.path.append(str_path)
-        ensure_dir_exists(new)
-        readme = os.path.join(new, 'README')
-        readme_src = os.path.join(get_ipython_package_dir(), u'config', u'profile', 'README')
-        if not os.path.exists(readme) and os.path.exists(readme_src):
-            shutil.copy(readme_src, readme)
-        for d in ('extensions', 'nbextensions'):
-            path = os.path.join(new, d)
-            try:
-                ensure_dir_exists(path)
-            except OSError as e:
-                # this will not be EEXIST
-                self.log.error("couldn't create path %s: %s", path, e)
-        self.log.debug("IPYTHONDIR set to: %s" % new)
+        if self.add_ipython_dir_to_sys_path:
+            str_path = os.path.abspath(new)
+            #sys.path.append(str_path)
+            ensure_dir_exists(new)
+            readme = os.path.join(new, 'README')
+            readme_src = os.path.join(get_ipython_package_dir(), u'config', u'profile', 'README')
+            if not os.path.exists(readme) and os.path.exists(readme_src):
+                shutil.copy(readme_src, readme)
+            for d in ('extensions', 'nbextensions'):
+                path = os.path.join(new, d)
+                try:
+                    ensure_dir_exists(path)
+                except OSError as e:
+                    # this will not be EEXIST
+                    self.log.error("couldn't create path %s: %s", path, e)
+            self.log.debug("IPYTHONDIR set to: %s" % new)
 
     def load_config_file(self, suppress_errors=IPYTHON_SUPPRESS_CONFIG_ERRORS):
         """Load the config file.


Code review comments:
diff_hunk: @@ -294,21 +303,22 @@ def _ipython_dir_changed(self, change):
             str_old = os.path.abspath(old)
             if str_old in sys.path:
                 sys.path.remove(str_old)
-        str_path = os.path.abspath(new)
-        sys.path.append(str_path)
-        ensure_dir_exists(new)
-        readme = os.path.join(new, 'README')
-        readme_src = os.path.join(get_ipython_package_dir(), u'config', u'profile', 'README')
-        if not os.path.exists(readme) and os.path.exists(readme_src):
-            shutil.copy(readme_src, readme)
-        for d in ('extensions', 'nbextensions'):
-            path = os.path.join(new, d)
-            try:
-                ensure_dir_exists(path)
-            except OSError as e:
-                # this will not be EEXIST
-                self.log.error("couldn't create path %s: %s", path, e)
-        self.log.debug("IPYTHONDIR set to: %s" % new)
+        if self.add_ipython_dir_to_sys_path:
+            str_path = os.path.abspath(new)
+            #sys.path.append(str_path)
path: IPython/core/application.py
text: @user1:
I'm not sure this is right - the append is unconditionally skipped, and the subdir creation presumably shouldn't be under this condition?

@author:
```suggestion
            sys.path.append(str_path)
```

@author:
Yeah, like a typo just before committing. 

I"d like to remove this option in the future, so my hope was to never create the dir if it is not needed.
Nowadays people can install extensions with pip so nothing should go in that dir maybe ?

I need to try with the cython extension see how it behaves.

diff_hunk: @@ -185,6 +185,15 @@ def _profile_changed(self, change):
                 get_ipython_package_dir(), u'config', u'profile', change['new']
         )
 
+    add_ipython_dir_to_sys_path = Bool(False,
+        """Should the IPython profile directory be added to sys path ?
+
+        This option was non-existing before IPython 8.0, and ipython_dir was added to
+        sys path to allow import of extensions present there. This was historical
+        baggage from when pip did not exist. This now default to false,
+        but can bet to true for legacy reasons
path: IPython/core/application.py
text: @user1:
```suggestion
        but can be set to true for legacy reasons.
```