-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
tox.ini: Add environments local-sudo-ubuntu-standard, etc. #30923
Comments
Author: Matthias Koeppe |
comment:2
Is it possible to put the sudo outside? I.e. |
comment:3
But we would only want to do the package installation as root, not the Sage build |
Commit: |
comment:5
Here's a first approximation. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:7
I think you forgot the confirmation Moreover, I was wondering if one should also read the Finally, there are problems that some of the packages are not found. First, one should probably add Other than that, looks good to me! Thanks a lot. |
comment:8
Yes, I'll add bootstrap I don't think we have But yes, handling For deadsnakes, there's #30217. This would not be the default, but a mix-in flavor. |
comment:9
On a second thought, do you really thing it's the job of tox to install system packages? I would feel more comfortable to put the code into a shell script that you can run as |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:11
Replying to @mkoeppe:
and/or change |
comment:12
Replying to @sagetrac-git:
Don't forget to change local-sudo-standard/maximal as well. |
comment:13
Replying to @tobiasdiez:
It's working well with tox, in particular the environment factors give a nice compact way of expressing different configurations. And I definitely do not want to add user-facing scripts to Sage that promises to install all necessary system packages. We deliberately only offer system package advice (at the end of But there is a lot of room for refactoring what is in |
comment:14
Replying to @tobiasdiez:
No, that line is also executed for these environments - by the magic of tox's factor conditions. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
This comment has been minimized.
This comment has been minimized.
Branch pushed to git repo; I updated commit sha1. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:20
Replying to @mkoeppe:
Sure, but it feels a bit like you are using tox for something it wasn't designed. Normally, in a tox setting, "environment" refers to a virtual python env, but for sage it is the complete system env including the os and system packages. I have the feeling a more general-purpose framework like invoke would be a better solution to manage these things.
I didn't meant it as a user-facing script, but only for developers (i.e don't ship it). |
comment:21
I've tested the current code in the wsl github workflow and it seems to work well: https://github.com/tobiasdiez/sage/actions/runs/365823571 |
comment:22
Sounds like a positive review? |
Reviewer: Tobias Diez, ... |
comment:23
Yes, in principle. But I cannot really judge the changes in the bash script. |
comment:25
ok |
Changed reviewer from Tobias Diez, ... to Tobias Diez, Dima Pasechnik |
comment:26
Thanks! |
comment:27
Is it possible to call only the system package install code via tox (i.e. don't run rest of build and tests)? That would be handy for #30371. |
comment:28
Replying to @tobiasdiez:
Yes, you can pass make targets to tox, e.g., |
comment:29
Thanks! That kind of worked. I had to install
Edit: Moreover,
Finally, Since using this tox command for ci was one of the motivations but doesn't really work at the moment without extra modifications, I'll set the ticket back to needs work. |
Changed branch from u/mkoeppe/tox_ini__add_environments_local_sudo_ubuntu_standard__etc_ to |
comment:32
Can you make a separate ticket for that? Once a ticket is in positive review I'll only unmerge it for critical bugs, not things that would also be good to have. |
Changed commit from |
comment:33
Sure! It's now #30944. Sorry if I broke any (unwritten) laws; I wasn't sure about resetting it to "needs work". |
These environments would just run
sudo apt-get ...
etc. to install (but not remove!) packages, for use at the user's own risk.To test (on an Ubuntu system):
or
CC: @tobiasdiez @dimpase
Component: porting
Author: Matthias Koeppe
Branch:
ff34897
Reviewer: Tobias Diez, Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/30923
The text was updated successfully, but these errors were encountered: