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

sage.misc.dist: Remove; deprecated in #30207 (2022) #37856

Merged
merged 1 commit into from
May 2, 2024
Merged

sage.misc.dist: Remove; deprecated in #30207 (2022) #37856

merged 1 commit into from
May 2, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 23, 2024 •
edited
Loading

Deprecated in:

📝 Checklist

  • 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 and checked the documentation preview.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Apr 23, 2024
@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 5a8b125; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe mkoeppe requested a review from jhpalmieri April 25, 2024 01:22
@mkoeppe mkoeppe changed the title src/sage/misc/dist.py: Remove; deprecated in #30207 (2022) sage.misc.dist: Remove; deprecated in #30207 (2022) Apr 25, 2024
@jhpalmieri
Copy link
Member

Looks good to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 25, 2024

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 28, 2024
sagemathgh-37856: sage.misc.dist: Remove; deprecated in sagemath#30207
0b2a719 
 (2022)

    
<!-- ^ 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". -->

Deprecated in:
- sagemath#30207 (2022)

### 📝 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.
- [ ] 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#37856
Reported by: Matthias Köppe
Reviewer(s):
@vbraun vbraun merged commit b14d3ec into sagemath:develop May 2, 2024
15 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 2, 2024
@soehms
Copy link
Member

soehms commented May 13, 2024

This broke our docker/Dockerfile for Docker Hub (see this workflow run) in line 234:

# Put scripts to start gap, gp, maxima, ... in /usr/bin
RUN sudo $SAGE_ROOT/sage --nodotsage -c "install_scripts('/usr/bin')"

Can I just delete these two lines or must they be replaced by something else?

@soehms soehms mentioned this pull request May 13, 2024
Merged
5 tasks
@mkoeppe mkoeppe deleted the sage_misc_dist_remove branch May 13, 2024 17:16
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 13, 2024

Sorry about missing this!
Perhaps $SAGE_ROOT/local/bin should just be prepended to PATH in this image.

@soehms soehms mentioned this pull request May 21, 2024
Merged
5 tasks
@soehms
Copy link
Member

soehms commented May 21, 2024

Sorry about missing this!

No problem!

Perhaps $SAGE_ROOT/local/bin should just be prepended to PATH in this image.

I'm implementing this in #38049, but I have to confess that it's not clear to me for what purpose we need to have $SAGE/local/bin in the system $PATH? I manually built the two missing Docker images for 10.4.beta5 and 10.4.beta6 and pushed them to Docker Hub. In the first image I removed the line with install_scripts. In beta6 I followed your suggestion. In both images, $SAGE_ROOT/local/bin is contained in $PATH within Sage. Accordingly, gap_console(), for example, works for both images. The only difference I've observed occurs when I execute a bash terminal in a container running on these images: In a container for beta6, I can run gap from the bash prompt what is not possible in a container on beta5. On the other hand, sage -gap works in both cases.

Therefore, it seems to me that the purpose of adding $SAGE_ROOT/local/bin to the system $PATH is not necessary for Sage to work. Leaving it out could eliminate a source of confusion.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 21, 2024

adding $SAGE_ROOT/local/bin to the system $PATH is not necessary for Sage to work

I agree.

I can't comment on what expectations people might have regarding what's available from bash in these Docker images.

@soehms
Copy link
Member

soehms commented May 22, 2024

I can't comment on what expectations people might have regarding what's available from bash in these Docker images.

I do not know that either. If in doubt, we should leave it as it is. I make the comment clearer on this.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
sagemathgh-38049: Fix broken docker/Dockerfile
7e76116 
<!-- ^ 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". -->

As mentioned in
sagemath#37856 (comment) the
`Dockerfile`  is broken by sagemath#37856.
This will be fixed in this PR.

More precisely we replace `install_scripts` by appending
`$SAGE_ROOT/local/bin` to the `bash` path variable. This does not affect
Sage but ensures easy access to `gap`,  `maxima`, ... in a `bash`
session (see
sagemath#37856 (comment) ff).

### 📝 Checklist

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

- [x] The title is concise and informative.
- [x] 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.
- [ ] 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#38049
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe
williamstein added a commit to sagemathinc/cocalc-compute-docker that referenced this pull request Jul 25, 2024
@williamstein
workaround sagemath/sage#37856
e555a92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhpalmieri jhpalmieri Awaiting requested review from jhpalmieri

Assignees

@mkoeppe mkoeppe

Labels
t: refactoring
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
4 participants
@mkoeppe @jhpalmieri @soehms @vbraun