Opened 7 years ago

Closed 6 years ago

#20529 closed enhancement (fixed)

Get rid of SAGE_ORIG_LD_LIBRARY_PATH

Reported by: Volker Braun Owned by:
Priority: major Milestone: sage-7.2
Component: build Keywords:
Cc: Erik Bray, Jeroen Demeyer Merged in:
Authors: Volker Braun Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 82d0a8c (Commits, GitHub, GitLab) Commit: 82d0a8c49fa59d57fc862d89dcdd974011d92cc0
Dependencies: Stopgaps:

Status badges

Description (last modified by Frédéric Chapoton)

the file src/sage/repl/interpreter.py uses the variable

SAGE_ORIG_LD_LIBRARY_PATH

which is not defined by default. This cause doctests failures on a patchbot not running in a bash shell.

Let us get rid of this variable and related code.

Example of log with failing doctests:

https://patchbot.sagemath.org/log/20240/Ubuntu/14.04/x86_64/3.16.0-71-generic/irma-atlas/2016-06-30%2011:02:50?short

Change History (29)

comment:1 Changed 7 years ago by Frédéric Chapoton

Branch: public/20529
Commit: 7d96aab446a1f636740ac024635d656c149f8f1a

New commits:

7d96aabremove call to SAGE_ORIG_LD_LIBRARY_PATH

comment:2 Changed 7 years ago by git

Commit: 7d96aab446a1f636740ac024635d656c149f8f1aad7050c27eed53b81c564f0aee8e88196b83d348

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

ad7050crestore empty libraries variable

comment:3 Changed 7 years ago by git

Commit: ad7050c27eed53b81c564f0aee8e88196b83d34882d0a8c49fa59d57fc862d89dcdd974011d92cc0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

82d0a8cGet rid of SAGE_ORIG_LD_LIBRARY_PATH

comment:4 Changed 7 years ago by Volker Braun

Authors: Volker Braun
Status: newneeds_review

IMHO a bit more drastic action is needed ;-)

comment:5 Changed 7 years ago by Frédéric Chapoton

Right, thanks for taking care of that. Let us wait for a bot report.

comment:6 Changed 7 years ago by Vincent Delecroix

Why SAGE_ORIG_LD_LIBRARY_PATH_SET was introduced in the first place?

comment:7 Changed 7 years ago by Vincent Delecroix

seems to go back to the old #975...

comment:8 Changed 7 years ago by Frédéric Chapoton

Hmm, sadly there will be no bot report, as this is not a "safe" ticket. And the "safe_only=False" mode of the bot is not something I would try myself, as it has not been used or taken care of for quite a long time.

comment:9 Changed 7 years ago by Frédéric Chapoton

ok, I have tested that on a linux machine, and all tests passed.

Maybe someone on Darwin could confirm ?

comment:10 Changed 7 years ago by Volker Braun

thats what bots are for...

comment:11 Changed 7 years ago by Volker Braun

Any progress?

comment:12 Changed 7 years ago by Frédéric Chapoton

No.

0) once again, the patchbots are not going to look at that, because it is not "safe". 1) besides, there are very few patchbots running, and not a single one on Darwin. 2) I have no Darwin machine to check it myself.

So no progress, no way.

comment:13 Changed 7 years ago by Volker Braun

The buildbot will test it on OSX

comment:14 Changed 7 years ago by Frédéric Chapoton

Hello !

Does this mean that I need to set a positive review before this is tested on Darwin ?

comment:15 Changed 7 years ago by Volker Braun

Yes. Related: Review is about reading the code, not about testing it by hand on multiple platforms.

comment:16 Changed 7 years ago by Frédéric Chapoton

Cc: Erik Bray Jeroen Demeyer added

There remains an "LD_LIBRARY_PATH" in the Cygwin section of src/bin/sage-env. But maybe this is still needed ? Anyway, this is way above my technical expertise. So, help is required, please!

comment:17 Changed 7 years ago by Erik Bray

This was added in #14380. It should still be there (library search paths are "weird" in Cygwin due to the use of Windows conventions for searching for DLLs--as for $LD_LIBRARY_PATH it doesn't normally need to be set but does affect dlopen() calls so we set it here).

comment:18 Changed 6 years ago by Frédéric Chapoton

Could please somebody review this ticket ? This prevents one of my patchbots from working correctly. I do not feel able to review this myself.

comment:19 Changed 6 years ago by Erik Bray

I would but the description of the ticket is empty and I have no context for this. Why for example is sage.repl.interpreter being removed entirely?

comment:20 in reply to:  19 Changed 6 years ago by Volker Braun

Replying to embray:

I would but the description of the ticket is empty and I have no context for this. Why for example is sage.repl.interpreter being removed entirely?

It is not if you look at the commit https://git.sagemath.org/sage.git/commit/?h=82d0a8c49fa59d57fc862d89dcdd974011d92cc0

This seems odd in the trac git diff viewer...

comment:21 Changed 6 years ago by Frédéric Chapoton

Description: modified (diff)

comment:22 Changed 6 years ago by Erik Bray

Weird, something is definitely amiss with the diff viewer....

comment:23 Changed 6 years ago by Erik Bray

So I guess it's safe to say nobody actually needs SAGE_ORIG_LD_LIBRARY_PATH anymore? In that case, looks good to me.

comment:24 Changed 6 years ago by Nils Bruin

I would expect that sage-native-execute (which is necessary for various interfaces and tools in sage) would need to restore the original LD_LIBRARY_PATH. See #9386.

comment:25 Changed 6 years ago by Volker Braun

We don't set LD_LIBRARY_PATH any more, which is why we don't need SAGE_ORIG_LD_LIBRARY_PATH

comment:26 Changed 6 years ago by Frédéric Chapoton

Eric, are you ready to set that to positive review ?

comment:27 Changed 6 years ago by Erik Bray

Reviewers: Erik Bray
Status: needs_reviewpositive_review

comment:28 Changed 6 years ago by Frédéric Chapoton

Thanks a lot.

comment:29 Changed 6 years ago by Volker Braun

Branch: public/2052982d0a8c49fa59d57fc862d89dcdd974011d92cc0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.