Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Fix restart mechanism for the Windows standalone installer (Installers) #20699

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

dalthviz
Copy link
Member

Description of Changes

Be sure to use the script the standalone installer uses in the shortcut to start and don't remove installer pkgs entry from the PYTHONPATH when starting Spyder.

Issue(s) Resolved

See #20658 (comment)

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

@ccordoba12 ccordoba12 changed the title PR: Fix restart mechanism for Windows standalone installer (Installer) PR: Fix restart mechanism for the Windows standalone installer (Installers) Mar 18, 2023
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small suggestion for you @dalthviz, otherwise looks good to me.

spyder/app/start.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

ccordoba12 commented Mar 18, 2023

@mrclary, it seems one of the patches for the Linux installer is not applying cleanly now. What can we do about that?

@mrclary
Copy link
Contributor

mrclary commented Mar 18, 2023

It is probably because there is a merge conflict with the patch. This should be resolved by rebasing the patch branch and resolving any conflicts. I'll take a look to be sure and fix any issues I find.

@mrclary
Copy link
Contributor

mrclary commented Mar 18, 2023

Regarding this PR, why does the Windows Spyder application require adding package paths to the user's PYTHONPATH? That seems unreliable as PYTHONPATH is intended for user customization and may be modified for any reason. Will the conda-based installer for Windows require use of PYTHONPATH as well?

@mrclary
Copy link
Contributor

mrclary commented Mar 18, 2023

Okay, there is a direct merge conflict between the patch and the changes in this PR.
My recommendation is to move forward with this PR and ignore the failing conda-based installer failure. After this PR is merged, I'll rebase the patch branch and resolve the conflict, verifying that the conda-based Linux installer successfully builds.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@dalthviz
Copy link
Member Author

why does the Windows Spyder application require adding package paths to the user's PYTHONPATH?

I think that's the way pynsist works. Checking the .pyw script that pynsist creates to launch an application you can see the addition of the packages dir to PYTHONPATH. I think pynsist also does some setup with .pth files which, from what I understand, require the addition of the pkgs path to the PYTHONPATH. Just in case, the Spyder-launch.pyw script is this:

#!python3.8
import sys, os
import site
scriptdir, script = os.path.split(os.path.abspath(__file__))
installdir = scriptdir  # for compatibility with commands
pkgdir = os.path.join(scriptdir, 'pkgs')
sys.path.insert(0, pkgdir)
# Ensure .pth files in pkgdir are handled properly
site.addsitedir(pkgdir)
os.environ['PYTHONPATH'] = pkgdir + os.pathsep + os.environ.get('PYTHONPATH', '')

# APPDATA should always be set, but in case it isn't, try user home
# If none of APPDATA, HOME, USERPROFILE or HOMEPATH are set, this will fail.
appdata = os.environ.get('APPDATA', None) or os.path.expanduser('~')

if 'pythonw' in sys.executable:
    # Running with no console - send all stdstream output to a file.
    kw = {'errors': 'replace'} if (sys.version_info[0] >= 3) else {}
    sys.stdout = sys.stderr = open(os.path.join(appdata, script+'.log'), 'w', **kw)
else:
    # In a console. But if the console was started just for this program, it
    # will close as soon as we exit, so write the traceback to a file as well.
    def excepthook(etype, value, tb):
        "Write unhandled exceptions to a file and to stderr."
        import traceback
        traceback.print_exception(etype, value, tb)
        with open(os.path.join(appdata, script+'.log'), 'w') as f:
            traceback.print_exception(etype, value, tb, file=f)
    sys.excepthook = excepthook



if __name__ == '__main__':
    from spyder.app.start import main
    main()

Will the conda-based installer for Windows require use of PYTHONPATH as well?

I don't think so since with the conda-based installer the installation is basically a conda env, right?

@mrclary
Copy link
Contributor

mrclary commented Mar 21, 2023

So pynsist creates the Spyder-launch.pyw script you listed?
It makes sense to me that pkgdir should be added to sys.path

sys.path.insert(0, pkgdir)
# Ensure .pth files in pkgdir are handled properly
site.addsitedir(pkgdir)

But I don't know why it is added to os.environ['PYTHONPATH']

os.environ['PYTHONPATH'] = pkgdir + os.pathsep + os.environ.get('PYTHONPATH', '')

Yes, the conda-based installers are exactly a conda installation, just in a non-standard location. You asked elsewhere whether we could use an nsist template for the conda-based installer, and I believe that we can. I don't know exactly what the template will do compared to the our existing Windows installer, e.g. will it generate the same Spyder-launch.pyw? I suppose we'll just have to cross that bridge when we get to it.

@dalthviz
Copy link
Member Author

@ccordoba12 @mrclary should we merge this one as it is, right?

@mrclary
Copy link
Contributor

mrclary commented Mar 22, 2023

@ccordoba12 @mrclary should we merge this one as it is, right?

I think so. I'll resolve conflicts with the conda-based installer patch branch after this PR is merged.
I did not intend to hijack or delay this PR with my most recent questions/comments regarding PYTHONPATH.

@dalthviz
Copy link
Member Author

dalthviz commented Mar 22, 2023

I think so. I'll resolve conflicts with the conda-based installer patch branch after this PR is merged.

👍

I did not intend to hijack or delay this PR with my most recent questions/comments regarding PYTHONPATH.

Not at all! And in fact, quite interesting questions to think about in order to understand better what we need to do to migrate from the old installers 👍

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dalthviz!

@ccordoba12 ccordoba12 merged commit ba0fbd8 into spyder-ide:5.x Mar 23, 2023
ccordoba12 added a commit that referenced this pull request Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants