Closed mkoeppe closed 1 year ago
Reviewer: Tobias Diez (-1), ...
You created a reusable infrastructure for something that is needed in one place and thus doesn't have to be reused. For this abstraction you are paying the usual costs that come with an additional level of indirection in terms of simplicity, discoverability, maintainability etc. I think its fair to criticize this as part of a review. So -1 from my side for the current solution to install gpgconf into the devcontainer.
There is always a trade off. I made the abstraction in full awareness of the indirection costs, and it has served the project well. Keeping the system package data in a central place goes a long way for maintainability and discoverability.
Replying to @tobiasdiez:
You created a reusable infrastructure for something that is needed in one place and thus doesn't have to be reused.
The package scripts are used for informing the user, for provisioning environments, and for generating documentation. Multiple uses, guaranteeing that things don't fall out of sync.
Replying to @mkoeppe:
Replying to @tobiasdiez:
You created a reusable infrastructure for something that is needed in one place and thus doesn't have to be reused.
The package scripts are used for informing the user, for provisioning environments, and for generating documentation. Multiple uses, guaranteeing that things don't fall out of sync.
I don't mean the general package infrastructure, but the _develop
package and in particular that gpgconf is installed there. Your advantages don't apply there: its not used to inform the user, its only used for the devcontainer, it is not displayed in the documentation, has only a single use and thus cannot fall out of sync. If in the future there might be the need to reuse it somewhere else then given the usecase one could decide about the right form of abstraction (e.g. bundling with other "devtools" or as an ordinary optional package or ...).
Replying to @tobiasdiez:
it is not displayed in the documentation
Yes it is, it's automatic.
Replying to @tobiasdiez:
If in the future [...] decide about the right form of abstraction (e.g. bundling with other "devtools" or as an ordinary optional package or ...).
It's not like these packages are mysterious VS Code-specific requirements. gpg is a typical dev tool that people use for signing commits, and ssh is also what trac users need to push.
Actually, from my use of repology for finding the gpgconf it seems that Debian is the only distro family that has it as a separate package. So one could consider to also package the full gpg in the case of Debian. That's a possible refinement.
Branch pushed to git repo; I updated commit sha1. New commits:
9e674ca | .devcontainer/Dockerfile: Add argument DOCKER_TAG |
d59144c | src/doc/en/developer/portability_testing.rst: Add section 'Using our pre-built Docker images published on ghcr.io' |
47f8b98 | src/doc/en/developer/portability_testing.rst: Add section 'Developing with our pre-built Docker images as devcontainers in VS Code' |
Given that we ask users to edit the devcontainer.json
file, perhaps we should only check in a template to avoid merge conflicts?
Description changed:
---
+++
@@ -1,3 +1,7 @@
https://code.visualstudio.com/docs/remote/containers
-Can be tested by starting a codespace at https://github.com/codespaces/new?hide_repo_select=true&ref=public%2Fbuild%2Fdevcontainer&repo=302607352&machine=standardLinux32gb&devcontainer_path=.devcontainer%2Fdevcontainer.json (might not be possible for everyone as codespaces is still in the test phase).
+We add two new sections in the developer's guide:
+https://47f8b984ef964a5aa34147393fbdc32e0dde88ad--sagemath-tobias.netlify.app/developer/portability_testing.html#using-our-pre-built-docker-images-published-on-ghcr-io
+
+Users who have access to GitHub Codespaces may also be able to test it by starting a codespace at https://github.com/codespaces/new?hide_repo_select=true&ref=public%2Fbuild%2Fdevcontainer&repo=302607352&machine=standardLinux32gb&devcontainer_path=.devcontainer%2Fdevcontainer.json
+
Perhaps it would also make sense to include another sample configuration for using sagemathinc/cocalc
as a devcontainer?
Branch pushed to git repo; I updated commit sha1. New commits:
0d573da | src/doc/en/developer/portability_testing.rst: Copy or symlink the devcontainer.json sample file |
This works, but getting /usr/local/sage/local/lib/libgmp.a: error adding symbols: archive has no index; run ranlib to add one
on cocalc
computop/sage has SAGE_ROOT set in the global environment. So this needs #33786.
Changed dependencies from #33851 to #33851, #33786
Branch pushed to git repo; I updated commit sha1. New commits:
60bcbec | README.md, src/doc/en/installation/source.rst: Update multi-user install |
2116f9f | README.md: Update final step 'symlink sage', add jupyter kernel step |
5c68335 | Merge #33787 |
252372d | SAGE_ROOT/sage: Unconditionally determine SAGE_ROOT from $0; no longer invite to edit this file |
01caa38 | SAGE_ROOT/sage: Actually unconditionally determine SAGE_ROOT from $0 |
b9c9764 | Merge #33786 |
9cd17f9 | .devcontainer/devcontainer-computop-sage.json: Stop it from injecting old SAGE_ROOT into every shell |
Branch pushed to git repo; I updated commit sha1. New commits:
9195e15 | .devcontainer/devcontainer-computop-sage.json: More workarounds |
Description changed:
---
+++
@@ -5,3 +5,6 @@
Users who have access to GitHub Codespaces may also be able to test it by starting a codespace at https://github.com/codespaces/new?hide_repo_select=true&ref=public%2Fbuild%2Fdevcontainer&repo=302607352&machine=standardLinux32gb&devcontainer_path=.devcontainer%2Fdevcontainer.json
+We also set up devcontainer configurations for the CoCalc and computop/sage Docker images.
+- https://github.com/3-manifolds/sagedocker/issues/2
+
Description changed:
---
+++
@@ -5,6 +5,6 @@
Users who have access to GitHub Codespaces may also be able to test it by starting a codespace at https://github.com/codespaces/new?hide_repo_select=true&ref=public%2Fbuild%2Fdevcontainer&repo=302607352&machine=standardLinux32gb&devcontainer_path=.devcontainer%2Fdevcontainer.json
-We also set up devcontainer configurations for the CoCalc and computop/sage Docker images.
+We also set up devcontainer configurations for the CoCalc, computop/sage, and sagemath/sagemath Docker images.
- https://github.com/3-manifolds/sagedocker/issues/2
Branch pushed to git repo; I updated commit sha1. New commits:
16fd4af | .devcontainer/devcontainer-archlinux-latest-downstream.json: Install pip, notebook |
Branch pushed to git repo; I updated commit sha1. New commits:
3c4929e | .devcontainer/devcontainer-conda-forge-latest-downstream.json: New |
Changed dependencies from #33851, #33786 to #33851, #33786, #33870
Work Issues: merge #33870, use _sagemath script package
Changed dependencies from #33851, #33786, #33870 to #33851, #33786, #33870, #33873
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
a2557d4 | build/pkgs/_sagemath/distros/debian.txt: Move debian info here from src/doc/en/installation/linux.rst |
3dd0d18 | build/pkgs/_sagemath/type: New |
f45f500 | Merge #33870 |
ab5a4f1 | build/bin/sage-get-system-packages: If system=auto, use sage-guess-package-system |
798f4d3 | build/bin/sage-print-system-package-command: If system=auto, use sage-guess-package-system |
ef249cc | build/bin/sage-print-system-package-command: New option --spkg |
d75da7e | tox.ini: Simplify using new options of sage-print-system-package-command |
780e747 | Merge #33873 |
d27f5e2 | .devcontainer/post_create.sh: Simplify using new options of sage-print-system-package-command |
df2216b | .devcontainer/devcontainer-archlinux-latest-downstream.json: Use spkg _sagemath |
Changed work issues from merge #33870, use _sagemath script package to none
Codespaces are not working as the devcontainer file is not recognized. According to https://github.blog/2022-04-20-codespaces-multi-repository-monorepo-scenarios/:
Codespaces now supports multiple devcontainer.json files inside of your .devcontainer directory, as long as they follow the pattern of .devcontainer/${DIR}/devcontainer.json
Interesting
I was looking for this feature in VS Code...
Branch pushed to git repo; I updated commit sha1. New commits:
bcf4302 | .devcontainer: Move devcontainer.json files to separate directories for Codespaces |
https://code.visualstudio.com/docs/remote/containers
We add two new sections in the developer's guide: https://47f8b984ef964a5aa34147393fbdc32e0dde88ad--sagemath-tobias.netlify.app/developer/portability_testing.html#using-our-pre-built-docker-images-published-on-ghcr-io
We also set up devcontainer configurations for the CoCalc and computop/sage Docker images, as well as downstream distribution packaging of Sage.
Tested
devcontainer.json
ofportability-ubuntu-jammy-standard
: builds well; runs welldevelop-docker-computop
: builds well; runs welldownstream-docker-cocalc
: builds well; runs well (except machines with the issue #32434)downstream-docker-computop
: builds well; runs welldownstream-archlinux-latest
: builds well; runs welldownstream-conda-forge-latest
: builds well; runs wellFollow-ups:
34363 Installation guide: Document installation on Windows via devcontainer
34286 Cookiecutter for Sage user projects with devcontainer
34403 GitHub Codespaces - may also be able to test it by starting a codespace at https:https://user-images.githubusercontent.com/5037600/216875765-6697bbf8-70f9-4acc-868e-9309db67be92.jpg/new?hide_repo_select=true&ref=public%2Fbuild%2Fdevcontainer&repo=302607352&machine=standardLinux32gb&devcontainer_path=.devcontainer%2Fdevcontainer.json
downstream-*
for more distributions (generate withtox
from the info intox.ini, use
_sagemath` package)develop-conda-forge-src-environment-dev
(https://doc.sagemath.org/html/en/installation/conda.html#using-conda-to-provide-all-dependencies-for-the-sage-library-experimental, similar to our gitpod configuration)portability-debian-buster-i386-standard
and fix itdevelop-docker-cocalc
(was: builds well (after increasing disk image space on the Docker daemon); fails to run)portability-centos-7-devtoolset-gcc_11-standard
(was: builds well; fails to run (exactly the same on WSL))downstream-docker-sagemath
(after #34242)portability-Dockerfile
as explained in comment:299Depends on #33873 Depends on #34352
CC: @tobiasdiez @dimpase @williamstein @culler @saraedum @kwankyu
Component: user interface
Author: Tobias Diez, Matthias Koeppe, Kwankyu Lee
Branch/Commit:
4affef2
Reviewer: Kwankyu Lee, Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/33671