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: Limit sphinx version to <7.4.0 to prevent bug with splash screen on Windows #22370

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Aug 22, 2024

Description of Changes

Limit sphinx version to <7.4.0. <=7.4.0 causes issue with Spyder's splash screen.

Also, only use matplotlib-base for installer runtime environment. This prevents installing unnecessary pyside and qt6-main packages.

Issue(s) Resolved

Fixes #22353

@ccordoba12 ccordoba12 changed the title PR: Limit sphinx version to <7.4.0 PR: Limit sphinx version to <7.4.0 to prevent bug with splash screen on Windows Aug 22, 2024
@ccordoba12 ccordoba12 added this to the v6.0rc2 milestone Aug 22, 2024
@ccordoba12
Copy link
Member

ccordoba12 commented Aug 22, 2024

@mrclary, I think we should include this fix in rc2 to test the installers before the final release. Do you agree?

@mrclary
Copy link
Contributor Author

mrclary commented Aug 22, 2024

Yes. Do you want me to include the changes to remove matplotlib?

@ccordoba12
Copy link
Member

Yes. Do you want me to include the changes to remove matplotlib?

Ok, please do that too.

@ccordoba12 ccordoba12 mentioned this pull request Aug 22, 2024
5 tasks
@mrclary mrclary marked this pull request as ready for review August 22, 2024 03:02
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.

Looks good to me, thanks @mrclary!

Note: The size of the Windows and Linux installers decreased by almost 100Mb by using matplotlib-base!

@@ -43,6 +43,7 @@ def setup_page(self):
features_group = QGroupBox(_("Additional features"))
math_box = self.create_checkbox(_("Render mathematical equations"),
'math')
# ??? Do we want to increase minimum sphinx requirement for Spyder?
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is a good idea. Perhaps increasing it to Sphinx 4 or 5 would be better, but I don't know enough about it to take an informed decision.

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 don't know enough either. It may be best to leave the lower limit where it currently is, at least for now.

…avoid installing unnecessary pyside and qt6 packages. tornado is already a dependent of ipykernel and jupyter-client.
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @mrclary ! I gave this PR Windows installer a local check and seems like the splash screen is working as expected 👍

If understood correctly, the sphinx dependency version constraint is only required for Windows right? If so, could it be worthy to only limit the Sphinx version on Windows? Also, could it be worthy to create a follow up issue to hopefully remove the upper constraint when the specific reason why this is happening is discovered, right @ccordoba12 @mrclary ?

@ccordoba12
Copy link
Member

If understood correctly, the sphinx dependency version constraint is only required for Windows right? If so, could it be worthy to only limit the Sphinx version on Windows?

I think it's easier to leave that constraint for all OSes for now. Then in 6.0.1 we can try to commit a proper fix.

Also, could it be worthy to create a follow up issue to hopefully remove the upper constraint when the specific reason why this is happening is discovered, right @ccordoba12 @mrclary ?

Good idea. Could you create one and tag it for 6.0.1? Thanks!

@dalthviz
Copy link
Member

Created #22374. Merging this to proceed with the release

@dalthviz dalthviz merged commit 0193cf6 into spyder-ide:master Aug 22, 2024
24 checks passed
@mrclary mrclary deleted the issue-22353 branch August 23, 2024 15:02
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.

Splash screen when running from installer shows partially (at least on Windows)
3 participants