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.

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

sage.env: Give values from sage_conf precedence over environment variables #34236

Open
mkoeppe opened this issue Jul 28, 2022 · 19 comments
Open

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jul 28, 2022

This change will make installations of Sage more robust.

Currently,

    /SAGE_ROOT_A/sage -sh -c '/SAGE_ROOT_B/venv/bin/python3 -c "import sage.env; print(sage.env.SAGE_ROOT)"'

will give SAGE_ROOT_A (bad), whereas

    /SAGE_ROOT_A/sage -sh -c '/SAGE_ROOT_B/venv/bin/sage -c "import sage.env; print(sage.env.SAGE_ROOT)"'

will give SAGE_ROOT_B (good).

Similarly to

CC: @jhpalmieri @kiwifb

Component: refactoring

Branch/Commit: u/mkoeppe/sage_env__give_values_from_sage_conf_precedence_over_environment_variables @ 4d20b92

Issue created by migration from https://trac.sagemath.org/ticket/34236

@mkoeppe mkoeppe added this to the sage-9.8 milestone Jul 28, 2022
@mkoeppe

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Aug 3, 2022

comment:2

I am a bit dubious when I see the title. I don't think you should override environment variables unless you absolutely have to.

What I think the ticket is about, and feel free to correct me, is that some variables, needed for sage's own operation, should be internal to sage and not exposed to be modifiable by environment variables. They should be private. If my interpretation is correct we need to start with a list of those variables that should be private.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

comment:3

Replying to @kiwifb:

What I think the ticket is about [...] is that some variables, needed for sage's own operation, should be internal to sage and not exposed to be modifiable by environment variables.

Well, that's a slightly stronger form than what I've written.

In what I wrote, the value would still be taken from the environment if it is not defined in sage_conf.

The stronger form would be fine for me as well.

@kiwifb
Copy link
Member

kiwifb commented Aug 3, 2022

comment:4

Thanks for the clarification. I guess from the point of view of what I have written, that makes all variables in sage_conf private.

From my distro point of view that may mean adding variables there that I did not care to have. Like SAGE_ROOT.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

comment:5

The stronger form can be implemented by just adding force=True.

    - ``force`` -- boolean (optional, default is ``False``). If
      ``True``, skip the environment variable and only use the
      fallbacks.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

comment:6

Some variables such as UNAME make no sense to even fetch from sage_conf. We could just set the value in sage.env.

@kiwifb
Copy link
Member

kiwifb commented Aug 3, 2022

comment:7

When you say in sage_conf I think of variables in this file https://github.com/sagemath/sage-prod/blob/develop/pkgs/sage-conf/_sage_conf/_conf.py.in - do you mean something else? I am asking because I am not sure where UNAME comes from.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

comment:8

Replying to @kiwifb:

When you say in sage_conf I think of variables in this file https://github.com/sagemath/sage-prod/blob/develop/pkgs/sage-conf/_sage_conf/_conf.py.in

Yes

I am asking because I am not sure where UNAME comes from.

It's set in the sage-env script and then again there's the fallback in sage.env.

@kiwifb
Copy link
Member

kiwifb commented Aug 3, 2022

comment:9

So, are you also wanting to cut some redundancies as part of the ticket? I guess it may make sense with some item.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

comment:11

Replying to @kiwifb:

So, are you also wanting to cut some redundancies as part of the ticket? I guess it may make sense with some item.

Well, many environment variables set in sage-env are completely useless, but I've been reluctant to remove them in case user scripts make use of them.

But by using force=True we can at least eliminate the useless dataflow from environment variables to sage.env variables


New commits:

9997254sage.env: Do not take UNAME, SAGE_VERSION, SAGE_DATE, SAGE_VERSION_BANNER, SAGE_VENV, SAGE_LIB, SAGE_EXTCODE from environment variables

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 3, 2022

Commit: 9997254

@kiwifb
Copy link
Member

kiwifb commented Aug 3, 2022

comment:12

Replying to @mkoeppe:

Replying to @kiwifb:

So, are you also wanting to cut some redundancies as part of the ticket? I guess it may make sense with some item.

Well, many environment variables set in sage-env are completely useless, but I've been reluctant to remove them in case user scripts make use of them.

Agree on the uselessness. sage-on-gentoo is sage-env less and so far I haven't had complains. If we had some kind of repo of user scripts, it would be an interesting thing to test.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

4a2fcd5sage.env: Clarify current behavior of var

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Changed commit from 9997254 to 4a2fcd5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Changed commit from 4a2fcd5 to bd08305

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

bd08305sage.env: Do not take SAGE_EXTCODE, SAGE_LOCAL, SAGE_SHARE, SAGE_DOC, SAGE_ARCHFLAGS from environment variables

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Changed commit from bd08305 to 4d20b92

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

2818890src/bin/sage-env: Fix typo in a comment
4d20b92sage.env: Check environment variable after sage_conf value, not before

@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants