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

Add file type association to Spyder shortcuts on all platforms. #171

Merged
merged 13 commits into from
Aug 20, 2024

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented May 13, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

conda/menuinst#117
conda/menuinst#183
conda/menuinst#225

Requires menuinst >= 2.1.2

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ccordoba12
Copy link
Contributor

This is great! Thanks @mrclary for working on it.

Quick question: how does this work on Windows?

@mrclary
Copy link
Contributor Author

mrclary commented May 13, 2024

I'm not sure how this works on Windows. I think @jaimergp is still working out some issues. Nevertheless, I wanted to create this draft PR so that I did not forget about it.

@mrclary mrclary marked this pull request as ready for review June 27, 2024 23:11
Copy link
Contributor

@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.

This looks good to me, thanks @mrclary!

I only have a quick question for you: did you manually test these changes on Windows, Linux and Mac? It's not clear if you already did it.

@mrclary
Copy link
Contributor Author

mrclary commented Jun 28, 2024

Not yet on Windows and Linux. But I'll do that and report back.

@mrclary mrclary marked this pull request as draft June 29, 2024 03:52
@mrclary
Copy link
Contributor Author

mrclary commented Jun 29, 2024

There appear to be some bugs with menuinst >= 2.1, so we could hold off on this for a bit more.

@mrclary mrclary force-pushed the file-type-association branch 2 times, most recently from dd592ab to 5d41802 Compare July 4, 2024 18:00
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@mrclary
Copy link
Contributor Author

mrclary commented Jul 28, 2024

@jaimergp, do you know when the next menuinst version will be released?

Array options for script and cat-ing to script are incompatible: ("-i" "''" "-e") works for cat-ing but not for script; ("-i" '' "-e") works for script but not cat-ing. For situations that require both script and cat-ing that must work for both GNU and BSD, the only solution is to use "-i.bak -e" and "rm *.bak"
Note: this results in a non-functioning application bundle, with nested application and broken executable stubs (conda/menuinst#179)
Note: this does not appear to establish file type association (conda/menuinst#185)
Note: I don't think this is a comprehensive list
conda-build copies cli-64.exe and spyder-script.py to the Scripts directory after bld.bat is run, so we must copy gui-64.exe such that it does not get clobbered, then use the post-link script to clean things up.

This work-around may be removed in the future if/when conda-build adds a feature to specify which executable stub to use. Hopefully, the resulting spyder.exe will also get the Spyder icon.
@ccordoba12 ccordoba12 marked this pull request as ready for review August 15, 2024 20:57
Copy link
Contributor

@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.

A few questions and suggestions for you @mrclary.

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/bld.bat Outdated
for /F "delims=. tokens=1" %%i in ("%PKG_VERSION%") do set PKG_MAJOR_VER=%%i
call :replace spyder-menu.json spyder-menu.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this replacement correct? Shouldn't it be replacing a .bak file, like in the line just below this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:replace reads the lines in the first file and replaces the __PKG_VERSION__ tokens with the major version number, then writes the lines to the second file. The default json file is for menuinst v2, so we just write back to the same file. Any *.json file will be executed by menuinst, so on the line below we also need to rename the spyder-menu-v1.json file so that menuinst does not try to execute on it. The post-link.bat script will determine whether v1 or v2 is in the base environment, and swap them if needed.

But perhaps it would be clearer to just have both files spyder-menu-v1.json and spyder-menu-v2.json, then let post-link.bat add .bak to the unnecessary file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the file names in the Menu directory should not change on install so that they will all be removed on uninstall. So it will be best to have spyder-menu-v1.json.bak and spyder-menu.json. Then copy v1 to spyder-menu.json (overwriting) only if necessary.

Comment on lines -25 to -27
del %SCRIPTS%\spyder_win_post_install.py
del %SCRIPTS%\spyder.bat
del %SCRIPTS%\spyder
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are not we removing these scripts now? I think they shouldn't be part of the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that they should not be part of the package. I removed these lines because these files were not being added to the %SCRIPTS% directory, so I assumed they were tech-debt. However, I can double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've confirmed that these files are not in the Scripts directory of the conda package (or anywhere else). Only the following:

Scripts/.spyder-post-link.bat
Scripts/gui-64.exe
Scripts/spyder-script.py
Scripts/spyder-script.pyw
Scripts/spyder.exe
Scripts/spyder.ico

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this script necessary now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required for Spyder to be listed as an application for file types in context menus. Windows looks at the shortcut executable for this, so when using pythonw.exe spyder-script.py, Spyder shows up as Python. In order to fix this, the shortcut must use the spyder.exe executable stub, but this executable stub looks for spyder-script.pyw rather than spyder-script.py.

See:
conda/menuinst#185 (comment)
conda/menuinst#185 (comment)
conda/menuinst#185 (comment)

recipe/spyder-script.pyw Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Contributor

This is ready to be merged, right @mrclary?

@mrclary
Copy link
Contributor Author

mrclary commented Aug 20, 2024

Yes, if you are happy with my responses to your comments.
There is also spyder-ide/spyder#22362 that should be ready.

@ccordoba12
Copy link
Contributor

Yes, if you are happy with my responses to your comments.

Yep, I am. Let's merge it then.

@ccordoba12 ccordoba12 merged commit 39475fe into conda-forge:dev Aug 20, 2024
26 checks passed
@mrclary mrclary deleted the file-type-association branch August 20, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants