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

ECL does not fully recover from relocation #11359

Closed
nbruin opened this issue May 20, 2011 · 32 comments
Closed

ECL does not fully recover from relocation #11359

nbruin opened this issue May 20, 2011 · 32 comments

Comments

@nbruin
Copy link
Contributor

nbruin commented May 20, 2011

CPPFLAGS are recorded by ECL for future use as default values for c::*cc-flags*. But we want Sage to be relocate-able, so no build paths should be hardcoded.

This ticket proposes various changes to the ECL spkg-install that may work.

See also #11348 and this sage-devel thread.

Component: packages: standard

Keywords: ecl hardcoded paths

Reviewer: Jeroen Demeyer

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

@nbruin
Copy link
Contributor Author

nbruin commented May 20, 2011

comment:1

One problem (perhaps the only one?) is that ECL uses the C compiler for compiling lisp (much like cython) and gives it include paths and library paths that it found out upon configuration. local/bin/sage-location is obviously not updating those. Example in a relocated "sage -ecl":

(defun f (x y) (+ x y))
(compile 'f) ;; fails
(setf c::*ecl-include-directory* (concatenate 'string (SI:GETENV "SAGE_ROOT") "/local/include/"))
(setf c::*ecl-library-directory* (concatenate 'string (SI:GETENV "SAGE_ROOT") "/local/lib/"))
(compile 'f) ;; now it works

There is a slight problem that the paths also end up in other variables (perhaps because of the way that sage calls configure when configuring ECL?):

> c::*cc-flags*

" -I/usr/local/sage/4.6.2-copy/local/include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -g -O2  -fPIC  -Dlinux"
> c::*ld-flags*

" -L/usr/local/sage/4.6.2-copy/local/lib  -lecl  -lgmp -lgc -ldl  -lm "

Since it is a bit unsanitary to have references to old locations lying around (one might link to wrong versions of libraries if the old location still exists!), we should probably strip those variables as well.

Possible solutions:

  • Patch ECL so that the above code is used to initialize these variables
  • Wrap our invocation of ECL so that we always update the values of these variables
  • equip local/bin/sage-location with the logic to update the relevant parts of ECL. That probably means recompiling (part of) ECL, because these values will be stored as a string in a ".fas"-file somewhere, which essentially a ".so".

@nbruin
Copy link
Contributor Author

nbruin commented May 20, 2011

comment:2

Confirmed! This seems to be indeed the only problem. I did the following in local/bin

$ mv ecl ecl.bin
$ cat <<<EOF >ecl
#!/bin/sh
exec ecl.bin \
  -eval '(setf c::*ecl-include-directory* (concatenate '"'"'string (SI:GETENV "SAGE_ROOT") "/local/include/"))'\
  -eval '(setf c::*ecl-library-directory* (concatenate '"'"'string (SI:GETENV "SAGE_ROOT") "/local/lib/"))' \
  "$@"
EOF
$ chmod a+x ecl

after which rebuilding maxima succeeds in a relocated position.

Note that this piece of code does not scrub c::cc-flags or c::ld-flags yet.

The definitions for the default values of these variables seem to live in
spkg/build/ecl-*/src/src/cmp/cmpdefs.lsp
so that is probably what we need to patch.

@nbruin
Copy link
Contributor Author

nbruin commented May 21, 2011

comment:3

The dirty c::*cc-flags* and c::*ld-flags are our own fault. In the ECL spkg-install we have the lines:

CPPFLAGS="$CPPFLAGS -I$SAGE_LOCAL/include"
LDFLAGS="$LDFLAGS -L$SAGE_LOCAL/lib"

but we are adding paths there, not flags! It needs the include path to find the boehm-gc headers, but our library search paths should already be set up. Can we instead just do

C_INCLUDE_PATH="$SAGE_LOCAL/include"
export C_INCLUDE_PATH

If the library really needs to be mentioned here, perhaps also a

LIBRARY_PATH="$SAGE_LOCAL/lib"
export LIBRARY_PATH

but I think we can leave that one out. With this I get an ECL where we obtain

> (require 'cmp)
> c::*cc-flags*

" -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -g -O2  -fPIC  -Dlinux"
> c::*ld-flags*

"  -lecl  -lgmp -lgc -ldl  -lm "

so no paths in the flags variables anymore! That just leaves figuring out where best to change the values of the ecl-...-directory variables.

@nbruin
Copy link
Contributor Author

nbruin commented May 21, 2011

comment:4

The following two fixes resolve the issues

Cleaning paths from ..-flags variables

In <ECL-PACKAGE>/spkg-install replace the lines

CPPFLAGS="$CPPFLAGS -I$SAGE_LOCAL/include"
LDFLAGS="$LDFLAGS -L$SAGE_LOCAL/lib"

by

C_INCLUDE_PATH="$SAGE_LOCAL/include"
export C_INCLUDE_PATH

(web documentation indicates that Sun Studio and IBM compilers also support this variable, so although it's not POSIX (neither is CPPFLAGS), it doesn't seem to be GNU exclusive.)

Making initialization of ecl-...-directory variables runtime

Patch <ECL-PACKAGE>/src/src/cmp/cmpdefs.lsp such that the lines

(defvar *ecl-include-directory* @includedir\@)
(defvar *ecl-library-directory* @libdir\@)

are replaced by

;;; The following code sets the default values of the include and library directories used for
;;; invocation of the C-compiler during compilation to values relative to ECL's idea
;;; of the current "system" directory. The logical pathname "SYS:" normally points to
;;; ".../local/lib/ecl", where "..." would normally be "/usr" (if ECL is installed in
;;; "/usr/local"). So, we strip off the last 2 components of "SYS:" and append "include" or
;;; "lib". This results in the values
;;;   ".../local/lib/"  and  ".../local/include/"  in the above example.
;;; Since this substitution happens at runtime, these values get adjusted appropriately if
;;; the ".../local" tree gets relocated.
(defvar *ecl-include-directory* (namestring (make-pathname :directory (append (butlast (pathname-directory (translate-logical-pathname "SYS:")) 2) '("include")))))
(defvar *ecl-library-directory* (namestring (make-pathname :directory (append (butlast (pathname-directory (translate-logical-pathname "SYS:")) 2) '("lib")))))

@nbruin
Copy link
Contributor Author

nbruin commented May 21, 2011

comment:5

Avoiding patching ECL

The role of defvar is 2-fold: It declares a variable to have dynamic scoping rather than lexical scoping and it sets the value only if the variable wasn't bound already. So, as long as we execute the two defvar statements

(defvar c::*ecl-include-directory* (namestring (make-pathname :directory (append (butlast (pathname-directory (translate-logical-pathname "SYS:")) 2) '("include")))))
(defvar c::*ecl-library-directory* (namestring (make-pathname :directory (append (butlast (pathname-directory (translate-logical-pathname "SYS:")) 2) '("lib")))))

before the compiler package is loaded, our values will persist. So, if one finds another file of lisp instructions that are guaranteed to be executed upon ECL startup (~/.eclrc doesn't work because maxima is built by calling ecl -norc) we're still good. I was not able to find such a file.

@kiwifb
Copy link
Member

kiwifb commented May 21, 2011

comment:6

So do we have to do two things here? Reworking spkg-install and the relocation script or just one of them is enough? And then at what stage?

I am ready to cut a new ecl spkg for 4.7.1 if needed. I could take the opportunity to update maxima to 5.24.0 at the same time.

@nbruin
Copy link
Contributor Author

nbruin commented May 21, 2011

comment:7

Replying to @kiwifb:

So do we have to do two things here? Reworking spkg-install

Yes, that and patch ECL.

and the relocation script

No changes to relocation script required. The patch to ECL means that the default values for the *ecl-...-library* variables are derived from a quantity that is already aware of ECL's runtime location.

So both changes are necessary and both are to the ecl spkg and both take effect at ECL build-time (well, the patch of course also changes ECL's runtime behaviour. Since a large part of building ECL consists of ECL compiling itself, the two overlap)

(the comment about avoiding patching ECL is just recording possible avenues someone might pursue if they feel really reluctant to patch ECL)

@kiwifb
Copy link
Member

kiwifb commented May 21, 2011

comment:8

I am for the less intrusive option but I am also pragmatic if it is far more practical to patch ecl itself.
Your option for not patching ecl would involve calling ecl before compiling any lisp code right? Which means before building maxima and sage/libs/ecl.pyx in sage if I understand correctly. The second one especially bother me so patching may be the best avenue.

@nbruin
Copy link
Contributor Author

nbruin commented May 21, 2011

comment:9

Actually, for sage/libs/ecl.pyx it's not so much of a problem, since our initialization of ecllib involves sending some instructions there anyway, so we could just add the defvars there. It would add a minuscule overhead for something that is unlikely to have any effect anyway: Who's going to use embedded ECL to compile something? That is one reason to prefer the patch to the compiler package already.

For fixing the behaviour of the ECL executable I haven't actually found a solution other than wrapping it with a shell script, which really does add considerable overhead.

Patching is definitely the cleaner solution, but will likely be with us in perpetuity, with all the extra maintenance of rebasing the patch with any ECL upgrade.

@kiwifb
Copy link
Member

kiwifb commented May 22, 2011

comment:10

OK. I am putting cutting a patched ecl according to your instruction on my TODO list for the week starting tomorrow. Like I said I'll probably cut a new maxima as well only
one non trivial doctest is being broken by 5.24.0 so it is easy work.

@nbruin
Copy link
Contributor Author

nbruin commented May 22, 2011

comment:11

Avoiding patching ECL in a future release

The development version of ECL has two configuration options that will allow avoiding patching ECL in the future. We'll be able to make a file /path/to/sage.lsp:

(in-package "SI")
(defun customized-boot ()
   (defparameter *ecl-include-directory* ...)
   (si::top-level t))

and then add the following options to ECL's configure:

--with-extra-files="/path/to/sage.lsp"
--with-init-form='(si::customized-boot)'

This option is not available in 11.1.1 yet.

@kiwifb
Copy link
Member

kiwifb commented May 22, 2011

comment:12

Replying to @nbruin:

Avoiding patching ECL in a future release

The development version of ECL has two configuration options that will allow avoiding patching ECL in the future. We'll be able to make a file /path/to/sage.lsp:

(in-package "SI")
(defun customized-boot ()
   (defparameter *ecl-include-directory* ...)
   (si::top-level t))

and then add the following options to ECL's configure:

--with-extra-files="/path/to/sage.lsp"
--with-init-form='(si::customized-boot)'

This option is not available in 11.1.1 yet.

Should we delay making a new spkg until it is released or do you think
we should still do it now with a note to check ecl at the next rev-bump?

@nbruin
Copy link
Contributor Author

nbruin commented May 23, 2011

comment:13

Replying to @kiwifb:

Should we delay making a new spkg until it is released or do you think
we should still do it now with a note to check ecl at the next rev-bump?

That depends on how urgently people think this needs fixing. Given that in practice, it only affects people wanting to build maxima on a moved Sage install and that they have a reasonable workaround of rebuilding ecl first (which builds quickly), I'd say this can wait. But if you're going to make a new ECL package anyway, I'd say patch the flaw, or at least the CCFLAGS thing.

@kiwifb
Copy link
Member

kiwifb commented May 23, 2011

comment:14

I am not in a particular hurry as I have other things I'd like to do. So unless there is pressure to do so. I'll wait for the next ecl.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

Author: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented May 30, 2013

comment:16

Attachment: ecl-12.12.1.p4.diff.gz

Whats with the CFLAGS discussion on the ticket? Not necessary any more? The new spkg doesn't address that, it just implements a workaround for broken include paths.

@jdemeyer
Copy link

comment:17

Replying to @vbraun:

The new spkg doesn't address that, it just implements a workaround for broken include paths.

True, but I guess this workaround is sufficient if it makes maxima build.

@nbruin
Copy link
Contributor Author

nbruin commented May 30, 2013

comment:18

I think the c-flags are still a problem. Try this

sage -ecl
> (defun f (x y) (+ x y))
> (compile 'f) ;; make sure the compiler package is loaded and initialized
>  c::*cc-flags*
" -I/usr/local/sage/5.9b4/local/include -I/usr/local/sage/5.9b4/local/include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -g -O2  -fPIC -Dlinux"
> c::*ld-flags*
" -L/usr/local/sage/5.9b4/local/lib -Wl,--rpath,/usr/local/sage/5.9b4/local/lib -L/usr/local/sage/5.9b4/local/lib -lecl  -lgmp -lgc -ldl  -lm "

I didn't check, but I assume these paths are still hardcoded and hence not adjusted by relocation. We're putting the "-I" and I think also the "-L" directives into those paths upon ecl config-and-build time. They are just not the right spot to put those. We'd be better off letting the environment be so that these things are found automatically, e.g. via setting the appropriate link paths (already done) and include search paths (for instance via C_INCLUDE_PATH).

Failing that, we need to patch ECL so that these paths get set relative to $SAGE_ROOT, evaluated at ecl invokation time rather than during ecl package build.

As explained above, we can now also avoid patching sage and instead configure sage to include our own customized ecl boot routine to set up the environment with the appropriate path information during ecl invokation.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 30, 2013

comment:19

Replying to @nbruin:

Cleaning paths from ..-flags variables

In <ECL-PACKAGE>/spkg-install replace the lines

CPPFLAGS="$CPPFLAGS -I$SAGE_LOCAL/include"
LDFLAGS="$LDFLAGS -L$SAGE_LOCAL/lib"

by

C_INCLUDE_PATH="$SAGE_LOCAL/include"
export C_INCLUDE_PATH

(web documentation indicates that Sun Studio and IBM compilers also support this variable, so although it's not POSIX (neither is CPPFLAGS), it doesn't seem to be GNU exclusive.)

For the record: In contrast to C_INCLUDE_PATH etc., CPPFLAGS (and CFLAGS etc.) are never read by the compiler / preprocessor; their names are just conventions used in Makefiles (and make's default rules).

@jdemeyer
Copy link

comment:21

Nils or Leif: can you give an example of an actual compilation which fails because of this? The wrong paths might not matter that much if the proper include and library files are found anyway through other means (e.g. environment variables such as CPATH and LIBRARY_PATH set in sage-env).

@nbruin
Copy link
Contributor Author

nbruin commented May 30, 2013

comment:22

Replying to @jdemeyer:

Nils or Leif: can you give an example of an actual compilation which fails because of this? The wrong paths might not matter that much if the proper include and library files are found anyway through other means (e.g. environment variables such as CPATH and LIBRARY_PATH set in sage-env).

It's pretty clear how you can make it fail:

  • build sage and ecl in location <ORIGINAL>
  • move sage to another location
  • put a bogus file <ORIGINAL>/local/include/ecl/config.h (or whatever headers are required) in the old location
    now trying to compile a file with ecl will use an invalid header. It's the usual risk of keeping references to uncontrolled locations in lookup tables. You may be exposed to arbitrary crap.

(I admit I haven't actually tried. It may be that with the symlink proposed on #14662 in place, the lookup happens at the right place prior to the place searched due to the -I directive. But that would really just be a fluke).

I'm a little surprised this suddenly gets marked as blocker when the issue was known for 2 years.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2013

comment:23

FWIW, C*PATH and LIBRARY_PATH are searched last, after any dirs specified on the command line.

Especially for libs, searching "random" folders is pretty delicate. (Think e.g. of bdists; we of course still have almost the same problem with R[UN]PATHs, although there's chrpath, but that wouldn't be applicable in this case.)

And it's perhaps not too uncommon to copy a Sage installation for upgrading, i.e., keeping the files at the original location.

@jdemeyer
Copy link

comment:24

Replying to @nbruin:

I'm a little surprised this suddenly gets marked as blocker when the issue was known for 2 years.

Mainly because the new maxima spkg at #14556 exposes it. But also because there is a known fix (althought perhaps not perfect).

@jdemeyer
Copy link

comment:25

Replying to @nbruin:

It's pretty clear how you can make it fail:

  • build sage and ecl in location <ORIGINAL>
  • move sage to another location
  • put a bogus file <ORIGINAL>/local/include/ecl/config.h (or whatever headers are required) in the old location
    now trying to compile a file with ecl will use an invalid header.

Fair enough, if you intentionally want to break it by providing a bogus <ORIGINAL>/local/include/ecl/config.h, you can. But my spkg fixes the problem for people who don't intentionally break their setup.

Note: an old ecl/config.h (as in the case for a copied install) is fine.

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Jun 5, 2013

Changed author from Jeroen Demeyer to none

@vbraun
Copy link
Member

vbraun commented Jun 5, 2013

Changed keywords from ecl relocation to ecl hardcoded paths

@vbraun
Copy link
Member

vbraun commented Jun 5, 2013

comment:26

I will review the workaround as part of #14662.

Leaving this ticket open until you figure out how to set paths correctly ;-)

@vbraun vbraun modified the milestones: sage-5.10, sage-5.11 Jun 5, 2013
@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

comment:31

Obsolete by the new binary packaging I guess.

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

4 participants