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

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

Closed
mkoeppe opened this issue Nov 11, 2020 · 36 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 11, 2020

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.

CC: @orlitzky @dimpase @jhpalmieri

Component: scripts

Keywords: sd111

Author: Michael Orlitzky

Branch/Commit: a63d256

Reviewer: Matthias Koeppe

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Nov 11, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Dependencies: #29850

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Commit: a318e9e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

comment:5

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

comment:6

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

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Remove unused env variable SAGE_SCRIPTS_DIR Repair SAGE_SCRIPTS_DIR discovery to make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables Nov 11, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

Changed commit from a318e9e to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

Changed dependencies from #29850 to #29951

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

Changed branch from u/mkoeppe/remove_unused_env_variable_sage_scripts_dir to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

comment:7

Any takers for this one?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

Changed author from Matthias Koeppe to none

@orlitzky
Copy link
Contributor

comment:8

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

comment:9

Replying to @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.

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

comment:10

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

Changed dependencies from #29951 to #29951, #29852

@orlitzky
Copy link
Contributor

comment:12

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

comment:13

Replying to @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.

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).

@orlitzky
Copy link
Contributor

comment:14

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

comment:15

Replying to @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.

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

@mkoeppe mkoeppe changed the title Repair SAGE_SCRIPTS_DIR discovery to make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables Make $SAGE_LOCAL/bin/sage work again from any directory, in an environment without SAGE_* variables, following symlinks Nov 15, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 20, 2020

Changed dependencies from #29951, #29852 to #29951, #29852, #30013

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 20, 2020

comment:17

Best to work on it on top of #30013

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

comment:18

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2020

comment:19

User bug report - https://groups.google.com/g/sage-devel/c/tejOsRxfC9w/m/ulKTsowHBAAJ

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2021

Changed dependencies from #29951, #29852, #30013 to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2021

comment:21

Replying to @orlitzky:

Copy/paste it is, then.

Got time to work on this for Sage 9.3?

@orlitzky
Copy link
Contributor

Author: Michael Orlitzky

@orlitzky
Copy link
Contributor

Branch: u/mjo/ticket/30888

@orlitzky
Copy link
Contributor

Commit: a63d256

@orlitzky
Copy link
Contributor

comment:22

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 21, 2021

comment:23

This works well, thanks very much.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 21, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 29, 2021

comment:25

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

@vbraun
Copy link
Member

vbraun commented Apr 2, 2021

Changed branch from u/mjo/ticket/30888 to a63d256

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

3 participants