Opened 4 years ago

Last modified 4 months ago

#11359 needs_work defect

ECL does not fully recover from relocation

Reported by: nbruin Owned by: GeorgSWeber
Priority: major Milestone: sage-6.4
Component: packages: standard Keywords: ecl hardcoded paths
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

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.

Attachments (1)

ecl-12.12.1.p4.diff (1.4 KB) - added by jdemeyer 19 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 4 years ago by nbruin

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

comment:2 Changed 4 years ago by nbruin

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.

comment:3 Changed 4 years ago by nbruin

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.

comment:4 follow-up: Changed 4 years ago by nbruin

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

comment:5 Changed 4 years ago by nbruin

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.

comment:6 follow-up: Changed 4 years ago by fbissey

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.

comment:7 in reply to: ↑ 6 Changed 4 years ago by nbruin

Replying to fbissey:

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)

comment:8 follow-up: Changed 4 years ago by fbissey

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.

comment:9 in reply to: ↑ 8 Changed 4 years ago by nbruin

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.

comment:10 Changed 4 years ago by fbissey

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.

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

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by fbissey

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?

comment:13 in reply to: ↑ 12 Changed 4 years ago by nbruin

Replying to fbissey:

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.

comment:14 Changed 4 years ago by fbissey

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.

comment:15 Changed 19 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Component changed from build to packages: standard
  • Description modified (diff)
  • Priority changed from minor to blocker
  • Status changed from new to needs_review

Changed 19 months ago by jdemeyer

comment:16 follow-up: Changed 19 months ago by vbraun

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.

comment:17 in reply to: ↑ 16 Changed 19 months ago by jdemeyer

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.

comment:18 Changed 19 months ago by nbruin

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.

comment:19 in reply to: ↑ 4 ; follow-up: Changed 19 months ago by leif

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

comment:20 in reply to: ↑ 19 Changed 19 months ago by nbruin

Replying to leif:

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

Ah, OK. For ECL it does have a meaning: the value gets inherited as hard-wired default value for c::*cc-flags*. There also is c::*ecl-include-directory*, so including -I directives in CPPFLAGS leads to superfluous includes passed to ecl compiler invocations. That would be one reason to try to get rid of it.

The other issue is that ecl by design hard-wires the default value of c::*ecl-include-directory*, which is the real obstacle to relocating ecl. We can change that by patching ecl (but since the current behaviour is likely not considered a bug for ecl, we'd be doing that indefinitely) or we can work around it by configuring ecl to include a different setting (as outlined above, with the --with-extra-files and --with-init-form directives). [Note that this change is only required if sage builds a "private ecl" (as it does normally). If sage were to rely on an ecl supplied via other means, we wouldn't be responsible for relocating it.]

comment:21 follow-up: Changed 19 months ago by 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).

Last edited 19 months ago by jdemeyer (previous) (diff)

comment:22 in reply to: ↑ 21 ; follow-ups: Changed 19 months ago by nbruin

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.

comment:23 Changed 19 months ago by leif

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.

comment:24 in reply to: ↑ 22 Changed 19 months ago by jdemeyer

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

comment:25 in reply to: ↑ 22 Changed 19 months ago by jdemeyer

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.

comment:26 Changed 19 months ago by vbraun

  • Authors Jeroen Demeyer deleted
  • Description modified (diff)
  • Keywords hardcoded paths added; relocation removed
  • Milestone changed from sage-5.10 to sage-5.11
  • Priority changed from blocker to major
  • Status changed from needs_review to needs_work

I will review the workaround as part of #14662.

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

comment:27 Changed 16 months ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:28 Changed 11 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:29 Changed 8 months ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:30 Changed 4 months ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.