Opened 2 years ago

Closed 22 months ago

#30888 closed enhancement (fixed)

Make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables, following symlinks

Reported by: Matthias Köppe Owned by:
Priority: blocker Milestone: sage-9.3
Component: scripts Keywords: sd111
Cc: Michael Orlitzky, Dima Pasechnik, John Palmieri Merged in:
Authors: Michael Orlitzky Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: a63d256 (Commits, GitHub, GitLab) Commit: a63d256e1f1968544822ff2af8b4ef146b7c9660
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

The feature introduced in #25486 (Discover SAGE_SCRIPTS_DIR to make $SAGE_LOCAL/bin/sage work from any directory, in an environment without SAGE_* variables) was killed by #30128 (enforce sourcing of sage-env-config before src/bin/sage-env).

We repair it, making sure that it works even if $0 is a symlink.

Change History (26)

comment:1 Changed 2 years ago by Matthias Köppe

Dependencies: #29850

comment:2 Changed 2 years ago by Matthias Köppe

Branch: u/mkoeppe/remove_unused_env_variable_sage_scripts_dir

comment:3 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe
Commit: a318e9e3c03bc466f966965df378ff4685ff6d12
Status: newneeds_review

Last 10 new commits:

c8e6910Merge tag '9.3.beta1' into t/29951/src_bin_sage_env__make_sage_env_config_and_sage_local_optional
9ab593cInstall sage-env-config with sage_conf
a361580build/pkgs/sage_conf/spkg-src: New
d02b597build/pkgs/sagelib/src/setup.py: Do not install sage-env-config
1b4bc99src/bin/sage: Only source sage-env-config if it exists
a1a6df5src/bin/sage: Use python3 etc. from PATH instead of using SAGE_LOCAL
d30b410build/pkgs/sage_conf/spkg-install: Build wheel manually
19f7eaesrc/bin/sage-env: Make sage-env-config optional
ecde605Merge branch 't/29850/install_sage_env_config_with_sage_conf' into t/30888/remove_unused_env_variable_sage_scripts_dir
a318e9esrc/bin/sage-env: Do not set SAGE_SCRIPTS_DIR

comment:4 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_info

comment:5 Changed 2 years ago by Matthias Köppe

Turns out that SAGE_SCRIPTS_DIR is unused because the feature introduced in #25486 (Discover SAGE_SCRIPTS_DIR to make $SAGE_LOCAL/bin/sage work from any directory, in an environment without SAGE_* variables) was killed by #30128, as part of the quest to remove bashisms.

comment:6 Changed 2 years ago by Matthias Köppe

Description: modified (diff)
Status: needs_infoneeds_work
Summary: Remove unused env variable SAGE_SCRIPTS_DIRRepair SAGE_SCRIPTS_DIR discovery to make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables

Unfortunately I did not notice it when I reviewed #30128.

comment:7 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe
Branch: u/mkoeppe/remove_unused_env_variable_sage_scripts_dir
Commit: a318e9e3c03bc466f966965df378ff4685ff6d12
Dependencies: #29850#29951

Any takers for this one?

comment:8 Changed 2 years ago by Michael Orlitzky

What kind of solution is acceptable here? I've lost track of the big picture, but it looks like all of the important ./configure-time settings are in sage-env-config, but local/bin/sage can't find sage-env-config because the path to it (determined during ./configure) is contained in sage-env-config.

Are we allowed to stick autoconf values directly into local/bin/sage (after renaming with an .in suffix), like

: ${SAGE_ROOT:=@abs_top_srcdir@}

which would set a default value of SAGE_ROOT at ./configure-time, but still allow custom values of SAGE_ROOT to be set in the environment?

comment:9 in reply to:  8 Changed 2 years ago by Matthias Köppe

Replying to mjo:

What kind of solution is acceptable here? I've lost track of the big picture, but it looks like all of the important ./configure-time settings are in sage-env-config, but local/bin/sage can't find sage-env-config because the path to it (determined during ./configure) is contained in sage-env-config.

The directory can be found from $0, with resolving symlinks. Similar to what used to be in sage-env, but not bash-specific because it's for an executed script rather than a sourced script.

Are we allowed to stick autoconf values directly into local/bin/sage (after renaming with an .in suffix), like

: ${SAGE_ROOT:=@abs_top_srcdir@}

which would set a default value of SAGE_ROOT at ./configure-time, but still allow custom values of SAGE_ROOT to be set in the environment?

No, because the sage script is installed by setup.py and we plan to make this work even when configure has not been run.

comment:10 Changed 2 years ago by Matthias Köppe

It could be done on top of #29852, just adding code to resolve a $0 symlink instead of using $0 directly.

comment:11 Changed 2 years ago by Matthias Köppe

Dependencies: #29951#29951, #29852

comment:12 Changed 2 years ago by Michael Orlitzky

We can fake realpath by copy & pasting the 70-line resolvelinks() function from sage-env, but I think that's pretty ugly:

  • It's copy/pasting a ton of code. We can't source sage-env for it, because we don't know where sage-env lives.
  • The function still doesn't work in some common cases, like hardlinks into the tree or symlinks out of the tree.
  • It's a waste of time recomputing the same paths every time "sage" is run, in slow shell script.

If setup.py installs the script, it knows the correct location. It could replace a placeholder at install-time as in e.g. https://git.launchpad.net/dkimpy-milter/tree/setup.py

comment:13 in reply to:  12 ; Changed 2 years ago by Matthias Köppe

Replying to mjo:

We can fake realpath by copy & pasting the 70-line resolvelinks() function from sage-env, but I think that's pretty ugly:

  • It's copy/pasting a ton of code. We can't source sage-env for it, because we don't know where sage-env lives.

I agree it's a bit ugly... that would be the third copy of the code (another one is in SAGE_ROOT/sage). Perhaps we can get rid of the copy in sage-env?

  • The function still doesn't work in some common cases, like hardlinks into the tree or symlinks out of the tree.

Hardlinks - granted.

What do you mean by "symlinks out of the tree"?

We would just document what is supported. The installation manual already has some guidance regarding "system wide installation" that needs updating anyway.

  • It's a waste of time recomputing the same paths every time "sage" is run, in slow shell script.

True, but perhaps the resolving code would only be run if the files cannot be found without resolving. Then it would impose the runtime penalty only if symlinks are in use.

If setup.py installs the script, it knows the correct location. It could replace a placeholder at install-time as in e.g. https://git.launchpad.net/dkimpy-milter/tree/setup.py

No, this is unfortunately not true. The final install location of the Python package is not known at setup.py time. When we build a wheel (which is the default installation mechanism in modern pip), the resulting wheel can be installed in any venv location (and the wheel format does not support post-install hooks of any kind).

comment:14 in reply to:  13 ; Changed 2 years ago by Michael Orlitzky

Replying to mkoeppe:

What do you mean by "symlinks out of the tree"?

If you move local/bin/sage elsewhere, and then create a symlink from local/bin/sage to that location. In that case, you don't want to use the absolute physical path of the script that's being run to find SAGE_ROOT, rather the absolute physical path of its original install location.

If setup.py installs the script, it knows the correct location. It could replace a placeholder at install-time as in e.g. https://git.launchpad.net/dkimpy-milter/tree/setup.py

No, this is unfortunately not true. The final install location of the Python package is not known at setup.py time. When we build a wheel (which is the default installation mechanism in modern pip), the resulting wheel can be installed in any venv location (and the wheel format does not support post-install hooks of any kind).

Ok, I don't know enough about how this works to propose anything else. Copy/paste it is, then.

comment:15 in reply to:  14 Changed 2 years ago by Matthias Köppe

Replying to mjo:

Replying to mkoeppe:

What do you mean by "symlinks out of the tree"?

If you move local/bin/sage elsewhere, and then create a symlink from local/bin/sage to that location. In that case, you don't want to use the absolute physical path of the script that's being run to find SAGE_ROOT, rather the absolute physical path of its original install location.

Right, let's just say that this is not supported.

comment:16 Changed 2 years ago by Matthias Köppe

Summary: Repair SAGE_SCRIPTS_DIR discovery to make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variablesMake $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables, following symlinks

comment:17 Changed 2 years ago by Matthias Köppe

Dependencies: #29951, #29852#29951, #29852, #30013

Best to work on it on top of #30013

comment:18 Changed 2 years ago by Matthias Köppe

Keywords: sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:20 Changed 2 years ago by Matthias Köppe

Dependencies: #29951, #29852, #30013

comment:21 in reply to:  14 Changed 23 months ago by Matthias Köppe

Replying to mjo:

Copy/paste it is, then.

Got time to work on this for Sage 9.3?

comment:22 Changed 23 months ago by Michael Orlitzky

Authors: Michael Orlitzky
Branch: u/mjo/ticket/30888
Commit: a63d256e1f1968544822ff2af8b4ef146b7c9660
Status: needs_workneeds_review

Further cleanup is possible, but I think this fixes the issue and makes the diff nice and clear.


New commits:

a63d256Trac #30888: resolve $0 links in src/bin/sage.

comment:23 Changed 23 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

This works well, thanks very much.

comment:24 Changed 22 months ago by Matthias Köppe

Priority: majorcritical

comment:25 Changed 22 months ago by Matthias Köppe

Priority: criticalblocker

Setting priority to blocker to bring this ticket to the attention of the release bot.

comment:26 Changed 22 months ago by Volker Braun

Branch: u/mjo/ticket/30888a63d256e1f1968544822ff2af8b4ef146b7c9660
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.