-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
Improve Windows installation instructions #37184
Conversation
ed71588
to
5a92df5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said that the installation on Ubuntu is currently broken. Is this specific to WSL?
cf4c1cd
to
904bb2c
Compare
What I wrote in the PR description was based on watching a student try it on his computer 2 weeks. I can't test it myself. Just now I tested the sagemath package on the ubuntu:jammy Docker image. It installs and tests OK as of today. In any case, the Ubuntu package is stuck at version 9.5, so for this reason alone it should not be our primary recommendation. |
904bb2c
to
55dafab
Compare
Thanks for the details. But this argument (especially your last point) applies to plain ubuntu as well. So I would prefer to simply change the how to install on ubuntu section to give priority to conda, and then leave the wsl section untouched. |
We do not have such a section. |
Sorry, I meant the "linux" section. |
That does not make much more sense either. The Linux section makes different recommendations based on the version of Sage that is found on the distribution. The problem that I am solving here in this PR is that there are too many indirections, each of which can confuse our potential users. That's what "streamlining" means in the PR description. |
The same would then also apply to Windows, as it is possible to use many different linux distros via wsl (as we also say in the docs). |
This is not an interesting discussion. |
Okay, let me see if I can summarize:
It will take me a little bit to test whether this actually works. But I do have access to a Windows computer if my son lets me on it 😄 However, I also don't want to get in the middle of the myriad meta-discussions going on around platforms etc. So I'm going to take the point of view that "Windows users probably need very explicit instructions, and shouldn't be asked to go to a scary Linux section" and not worry about Linux instructions, which however certainly might need updating as well. Technical question: do we expect that once |
Yes, that's also my viewpoint here. If not for anything else, then certainly for keeping the focus of the PR as narrow as possible. After all, even with this deliberately narrow focus, it has been sitting for 3 weeks.
No, |
I'm setting it to blocker because it needs to make its way in the stable release. |
I'll take a look once I get access to a Windows machine. By the way, any sense on why the Ubuntu is stuck on such an old version? (Probably this was discussed ad nauseam elsewhere, sorry.) |
I think maintainer burnout + possibly a problem with brial (hence #36380) |
Removing blocker as there are problems with other PRs, see #37428 for more details. |
I have merged #37588 (thanks @williamstein for testing that) and updated the instructions from there. I'm setting back to "blocker" because this really should be merged in 10.3. |
@kcrisman With these changes merged, do you want to test again, or can we merge? |
…orge <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Based on the discussion in https://groups.google.com/g/sage- devel/c/GaQHdu0Q6UU and sagemath#37184, we now follow the offical recommendation and use Miniforge over the soft- deprecated Mambaforge. Since we are exclusively testing on miniforge, this change also aligns the docs with the ci. Moreover, I've removed the recommendation to install mamba (using `conda install mamba`) which is not recommended according to the [offical docs](https://mamba.readthedocs.io/en/latest/installation/mamba- installation.html#existing-conda-install-not-recommended). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37588 Reported by: Tobias Diez Reviewer(s): Matthias Köppe, roed314, William Stein
I'm posting to record a vote of -1 on behalf of Tobias Diez. |
Tobias added the "disputed" label and has requested that it be removed, so I am doing so. Also, based on the policy stated at https://groups.google.com/g/sage-devel/c/XDvKkMRoDk4/m/0yrtdKkGAwAJ, "if there is disagreement about whether a PR should be marked as a blocker, mark it as critical instead." So I am marking this as critical. |
How can there be a vote if the PR is not "disputed" any more? |
In my opinion, if it's not disputed, "-1" votes have no clear impact, other than to let developers know that someone has some objections. |
Ideally someone should test again, though William's other testing is good. I just worry about Windows being wonky. However, I do vote +1 for this change overall, to be clear. Comments on testing:
Addressing the issue implicitly raised, this comment suggests that, while Tobias is currently voting -1 due to duplication, it might just as easily be possible to have a quick followup PR that then uses some standard rst tool to remove that duplication. I don't really care if it happens here or there, but it does seem helpful in terms of preventing doc rot. |
Sorry for missing this. Keeping the documentation source a simple .rst file (instead of generating it somehow from some template file so that we could refer to a variable) -- this is to keep the structure of our documentation sources as simple and straightforward as possible -- this serves the contributors to the documentation. The cost of this (= the cost of updating this occurrence) is low, as the update happens only about once a year, and the places to change are easy to find, in particular for those who know how to make the updates of the supported Python versions. |
FWIW, I recommend not to take such rationalizations at face value. |
Sorry for missing this.
No problem.
The cost of this (= the cost of updating this occurrence) is low, as the
update happens only about once a year, and the places to change are easy to
find, in particular for those who know how to make the updates of the
supported Python versions.Message ID:
***@***.***>
Okay, I guess we do have to search for that every so often ...
|
Tobias is currently voting -1 due to duplication
FWIW, I recommend not to take such rationalizations at face value.
Thankfully, I have the margin to do so, though I understand why others
might not.
… Message ID: ***@***.***>
|
…y miniforge installer
Done in d3a2a16 |
…ection, point to Launching
Done in 4eacb7b, please take a look. |
Documentation preview for this PR (built with commit 4eacb7b; changes) is ready! 🎉 |
Thanks for addressing all these concerns. While I still hope for an nth iteration of a double click installer, this is fairly painless as such things go in our Windows ecosystem experience. |
@kcrisman Thanks for the review! |
@vbraun Please merge. Can't set to blocker because policy. |
We streamline the installation instructions for Windows by including conda-forge instructions.
Sage is stuck at version 9.5 in Ubuntu, the default Linux distribution on WSL, so this should not be our primary recommendation.
Preview: https://deploy-preview-37184--sagemath.netlify.app/html/en/installation/
Alternatives / follow-ups:
📝 Checklist
⌛ Dependencies