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.

Sign up for GitHub

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

Jump to bottom

Get rid of SAGE_ORIG_LD_LIBRARY_PATH #20529

Closed
vbraun opened this issue May 1, 2016 · 34 comments
Closed

Get rid of SAGE_ORIG_LD_LIBRARY_PATH #20529

vbraun opened this issue May 1, 2016 · 34 comments
Labels
c: build p: major / 3 t: enhancement
Milestone
sage-7.2

Comments

@vbraun
Copy link
Member

vbraun commented May 1, 2016

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

CC: @embray @jdemeyer

Component: build

Author: Volker Braun

Branch/Commit: 82d0a8c

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/20529
@vbraun vbraun added this to the sage-7.2 milestone May 1, 2016
@fchapoton
Copy link
Contributor

New commits:

7d96aabremove call to SAGE_ORIG_LD_LIBRARY_PATH

@fchapoton
Copy link
Contributor

Commit: 7d96aab

@fchapoton
Copy link
Contributor

Branch: public/20529

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2016

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

ad7050crestore empty libraries variable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2016

Changed commit from 7d96aab to ad7050c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2016

Changed commit from ad7050c to 82d0a8c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2016

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

82d0a8cGet rid of SAGE_ORIG_LD_LIBRARY_PATH

@vbraun
Copy link
Member Author

vbraun commented May 1, 2016

Author: Volker Braun

@vbraun
Copy link
Member Author

vbraun commented May 1, 2016

comment:4

IMHO a bit more drastic action is needed ;-)

@fchapoton
Copy link
Contributor

comment:5

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

@videlec
Copy link
Contributor

videlec commented May 1, 2016

comment:6

Why SAGE_ORIG_LD_LIBRARY_PATH_SET was introduced in the first place?

@videlec
Copy link
Contributor

videlec commented May 1, 2016

comment:7

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

@fchapoton
Copy link
Contributor

comment:8

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.

@fchapoton
Copy link
Contributor

comment:9

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

Maybe someone on Darwin could confirm ?

@vbraun
Copy link
Member Author

vbraun commented May 3, 2016

comment:10

thats what bots are for...

@vbraun
Copy link
Member Author

vbraun commented May 6, 2016

comment:11

Any progress?

@fchapoton
Copy link
Contributor

comment:12

No.

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

So no progress, no way.

@vbraun
Copy link
Member Author

vbraun commented May 8, 2016

comment:13

The buildbot will test it on OSX

@fchapoton
Copy link
Contributor

comment:14

Hello !

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

@vbraun
Copy link
Member Author

vbraun commented May 8, 2016

comment:15

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

@fchapoton
Copy link
Contributor

comment:16

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!

@embray
Copy link
Contributor

embray commented May 9, 2016

comment:17

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

@fchapoton
Copy link
Contributor

comment:18

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

@embray
Copy link
Contributor

embray commented Jul 7, 2016

comment:19

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?

@vbraun
Copy link
Member Author

vbraun commented Jul 7, 2016

comment:20

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://github.com/sagemath/sagetrac-mirror/commits/82d0a8c49fa59d57fc862d89dcdd974011d92cc0

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

@fchapoton

This comment has been minimized.

Sign in to view
@embray
Copy link
Contributor

embray commented Jul 7, 2016

comment:22

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

@embray
Copy link
Contributor

embray commented Jul 7, 2016

comment:23

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

@nbruin
Copy link
Contributor

nbruin commented Jul 7, 2016

comment:24

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.

@vbraun
Copy link
Member Author

vbraun commented Jul 7, 2016

comment:25

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

@fchapoton
Copy link
Contributor

comment:26

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

@embray
Copy link
Contributor

embray commented Jul 11, 2016

Reviewer: Erik Bray

@fchapoton
Copy link
Contributor

comment:28

Thanks a lot.

@vbraun
Copy link
Member Author

vbraun commented Jul 12, 2016

Changed branch from public/20529 to 82d0a8c

@vbraun vbraun closed this as completed in d3721f6 Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: build p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-7.2
Development

No branches or pull requests
5 participants
@embray @vbraun @nbruin @fchapoton @videlec