Opened 4 years ago

Closed 4 years ago

#20529 closed enhancement (fixed)

Get rid of SAGE_ORIG_LD_LIBRARY_PATH

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

Description (last modified by 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 4 years ago by chapoton

  • Branch set to public/20529
  • Commit set to 7d96aab446a1f636740ac024635d656c149f8f1a

New commits:

7d96aabremove call to SAGE_ORIG_LD_LIBRARY_PATH

comment:2 Changed 4 years ago by git

  • Commit changed from 7d96aab446a1f636740ac024635d656c149f8f1a to ad7050c27eed53b81c564f0aee8e88196b83d348

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

ad7050crestore empty libraries variable

comment:3 Changed 4 years ago by git

  • Commit changed from ad7050c27eed53b81c564f0aee8e88196b83d348 to 82d0a8c49fa59d57fc862d89dcdd974011d92cc0

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 4 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

IMHO a bit more drastic action is needed ;-)

comment:5 Changed 4 years ago by chapoton

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

comment:6 Changed 4 years ago by vdelecroix

Why SAGE_ORIG_LD_LIBRARY_PATH_SET was introduced in the first place?

comment:7 Changed 4 years ago by vdelecroix

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

comment:8 Changed 4 years ago by 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 4 years ago by chapoton

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

Maybe someone on Darwin could confirm ?

comment:10 Changed 4 years ago by vbraun

thats what bots are for...

comment:11 Changed 4 years ago by vbraun

Any progress?

comment:12 Changed 4 years ago by 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 4 years ago by vbraun

The buildbot will test it on OSX

comment:14 Changed 4 years ago by chapoton

Hello !

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

comment:15 Changed 4 years ago by vbraun

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

comment:16 Changed 4 years ago by chapoton

  • Cc embray jdemeyer 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 4 years ago by embray

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 4 years ago by 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 follow-up: Changed 4 years ago by 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?

comment:20 in reply to: ↑ 19 Changed 4 years ago by vbraun

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 4 years ago by chapoton

  • Description modified (diff)

comment:22 Changed 4 years ago by embray

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

comment:23 Changed 4 years ago by embray

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 4 years ago by nbruin

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 4 years ago by vbraun

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

comment:26 Changed 4 years ago by chapoton

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

comment:27 Changed 4 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

comment:28 Changed 4 years ago by chapoton

Thanks a lot.

comment:29 Changed 4 years ago by vbraun

  • Branch changed from public/20529 to 82d0a8c49fa59d57fc862d89dcdd974011d92cc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.