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.

Sign up for GitHub

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

Jump to bottom

Make jmol optional #31027

Closed
mkoeppe opened this issue Dec 8, 2020 · 11 comments · Fixed by #38504
Closed

Make jmol optional #31027

mkoeppe opened this issue Dec 8, 2020 · 11 comments · Fixed by #38504
Labels
c: build c: graphics c: packages: standard p: major / 3 t: enhancement
Milestone
sage-10.5

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 8, 2020 •
edited
Loading

(split out from #30315)

We propose to downgrade jmol to optional.

It used to be needed for

See discussion: ​https://groups.google.com/g/sage-devel/c/qKqTmLzHAbg/m/6YxQ_qWUBQAJ
and #33507 for alternative technologies for creating 3D graphics for documentation and sagetex

Depends on #33507
Depends on #33534

CC: @kcrisman @novoselt @jcamp0x2a @antonio-rojas @kiwifb @seblabbe @egourgoulhon @dimpase

Component: build

Author: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/31027
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

Branch: u/mkoeppe/make_jmol_optional

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

Changed dependencies from #30315 to #30315, #31020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

Commit: ae970c2

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

Last 10 new commits:

0ed256cbuild/make/Makefile.in: Compute SAGE_CHECK_... earlier, before evaluating dependencies
1f714c1build/pkgs/sagetex/dependencies: Conditionalize order-only deps on SAGE_CHECK_sagetex
6a549d9Merge branch 't/31020/build_make_makefile_in__fix_sage_check_logic__conditionalize_sagetex_dependencies_on_sage_check' into t/30315/switch_jsmol_to_jupyter_jsmol__make_jmol_optional
3a45d72build/pkgs/jupyter_jsmol/spkg-install.in: Do not rebuild the javascript file
a8e0364Specify bdist temp folder for WSL compatibility
a3077c7Use mktemp
c5a14a3build/bin/sage-dist-helpers (sdh_setup_bdist_wheel): New
fb10429build/pkgs/*/spkg-install.in: Use new function sdh_setup_bdist_wheel
32bf463Merge branch 't/31002/public/build/bdist_wsl' into t/30315/switch_jsmol_to_jupyter_jsmol__make_jmol_optional
ae970c2build/pkgs/jupyter_jsmol/spkg-install.in: Use new function sdh_setup_bdist_wheel

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 16, 2022

Changed dependencies from #30315, #31020 to #33513

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

Changed dependencies from #33513 to #33507, #33534

@mkoeppe

This comment has been minimized.

Sign in to view
@orlitzky
Copy link
Contributor

The docbuild is already pseudo-optional via --disable-doc. I think both sagetex and jmol are good candidates for being pseuso-optional as well: nothing else in sage needs sagetex at all, and nothing bad happens to a normal user who doesn't have jmol. Some other viewer is used by default, and if you try to force jmol, you get an error. OK.

If we add --disable-sagetex and --disable-jmol flags, we could turn jmol off right now when both --disable-doc and --disable-sagetex are given. This would be merely a matter of accounting because, again, sage itself is fine without either sagetex or jmol.

(SageTeX should be completely optional because it's independent of everything, but we'll never get ML consensus to demote it.)

@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 5, 2024
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 12, 2024

Apparently (tested with #38504) the sagetex test suite no longer needs jmol to pass.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 12, 2024

Also docbuild (html and pdf) apparently no longer depends on jmol.
So let's go forward with demoting jmol to optional - #38504.

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 8, 2024
sagemathgh-38504: Demote jmol to optional
c4737ab 
    
<!-- ^ 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". -->

- Resolves sagemath#31027

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ 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#38504
Reported by: Matthias Köppe
Reviewer(s): Nathan Dunfield
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 8, 2024
sagemathgh-38504: Demote jmol to optional
26dc962 
    
<!-- ^ 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". -->

- Resolves sagemath#31027

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ 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#38504
Reported by: Matthias Köppe
Reviewer(s): Nathan Dunfield
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 10, 2024
sagemathgh-38504: Demote jmol to optional
17bf875 
    
<!-- ^ 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". -->

- Resolves sagemath#31027

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ 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#38504
Reported by: Matthias Köppe
Reviewer(s): Nathan Dunfield
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 12, 2024
sagemathgh-38504: Demote jmol to optional
4926b6c 
    
<!-- ^ 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". -->

- Resolves sagemath#31027

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ 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#38504
Reported by: Matthias Köppe
Reviewer(s): Nathan Dunfield
@vbraun vbraun closed this as completed in 9a30984 Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: build c: graphics c: packages: standard p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-10.5
Development

Successfully merging a pull request may close this issue.

Demote jmol to optional mkoeppe/sage
2 participants
@orlitzky @mkoeppe