Skip to content

Fix broken docker/Dockerfile #38049

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
Merged
merged 2 commits into from
May 25, 2024
Merged

Fix broken docker/Dockerfile #38049

merged 2 commits into from
May 25, 2024

Conversation

soehms
Copy link
Member

@soehms soehms commented May 21, 2024 •
edited
Loading

As mentioned in #37856 (comment) the Dockerfile is broken by #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 #37856 (comment) ff).

📝 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

@soehms soehms mentioned this pull request May 21, 2024
Merged
5 tasks
@github-actions GitHub Actions
Copy link

github-actions bot commented May 21, 2024 •
edited
Loading

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

@soehms soehms mentioned this pull request May 22, 2024
Merged
5 tasks
@soehms soehms marked this pull request as ready for review May 22, 2024 16:57
@soehms soehms requested review from mkoeppe and saraedum May 22, 2024 16:58
mkoeppe
mkoeppe approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM.

@soehms
Copy link
Member Author

soehms commented May 22, 2024

LGTM.

Thanks!

For the record: Tested in this run after merging with #37986.

@vbraun vbraun merged commit 7e76116 into sagemath:develop May 25, 2024
@soehms
Copy link
Member Author

soehms commented Jun 5, 2024

It seems that this didn't work as expected see this run for beta7 and this one for beta8, explicitly in the first case:

#16 64.41 [sagemath_doc_html-none] [spkg-install] sage -advanced > en/reference/repl/options.txt
#16 64.41 [sagemath_doc_html-none] [spkg-install] Traceback (most recent call last):
#16 64.41 [sagemath_doc_html-none] [spkg-install]   File "<frozen runpy>", line 198, in _run_module_as_main
#16 64.41 [sagemath_doc_html-none] [spkg-install]   File "<frozen runpy>", line 88, in _run_code
#16 64.41 [sagemath_doc_html-none] [spkg-install]   File "/home/sage/sage/local/var/lib/sage/venv-python3.11.8/lib/python3.11/site-packages/sage_docbuild/__main__.py", line 66, in <module>
#16 64.41 [sagemath_doc_html-none] [spkg-install]     from sage.env import SAGE_DOC_SRC
#16 64.41 [sagemath_doc_html-none] [spkg-install] ModuleNotFoundError: No module named 'sage'
#16 64.41 [sagemath_doc_html-none] [spkg-install] Error: 'sage --docbuild --all-documents' failed
#16 64.41 [sagemath_doc_html-none] [spkg-install] make[5]: *** [Makefile:41: doc-inventory-reference] Error 1

In the run for beta4 the module still was found.

@mkoeppe, do you have any idea what is missing?

@soehms soehms deleted the fix_broken_dockerfile branch June 5, 2024 18:42
@soehms soehms mentioned this pull request Jun 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saraedum saraedum Awaiting requested review from saraedum

1 more reviewer

@mkoeppe mkoeppe mkoeppe approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: docker p: blocker / 1 v: minimal
Projects
None yet
Milestone
sage-10.4
Development

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