Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21501 closed enhancement (fixed)

Allow SAGE_LOCAL to be customized

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.5
Component: build Keywords:
Cc: mkoeppe Merged in:
Authors: Jeroen Demeyer Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 64e697d (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This ticket allows a $SAGE_LOCAL directory different from $SAGE_ROOT/local.

The idea is that #21479 would set $SAGE_LOCAL to the --prefix chosen by the user.

Change History (21)

comment:1 follow-up: Changed 3 years ago by felixs

that seems to make sense. but it's dangerous really. SAGE_LOCAL is an internal directory and an implementation detail currently.

if you expose this, users will start expecting stuff in SAGE_LOCAL created by the build process. an ideal build process does not (will not/should not) use SAGE_LOCAL. any transition to fixing this will heavily interfere with user experience (only if they override SAGE_LOCAL, of course).

ok, now i explained why "set prefix==SAGE_LOCAL" is probably not a good idea. and here, an alternative, "expose SAGE_LOCAL", appears to be much worse... hmm

there's a problem here. can you please give some more insight on what you are trying to achieve with the customized SAGE_LOCAL?

thanks

comment:2 in reply to: ↑ 1 Changed 3 years ago by jdemeyer

  • Description modified (diff)

Replying to felixs:

SAGE_LOCAL is an internal directory and an implementation detail currently.

I am not changing that. I still consider it an implementation detail.

can you please give some more insight on what you are trying to achieve with the customized SAGE_LOCAL?

I am allowing Sage to be installed in a custom directory, which is not $SAGE_ROOT/local.

comment:3 follow-up: Changed 3 years ago by felixs

I am allowing Sage to be installed in a custom directory,

SAGE_LOCAL cannot be the installation directory *and* an implementation detail.

but you seem to have your own definition for both these terms, rendering this discussion pointless.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by jdemeyer

Replying to felixs:

I am allowing Sage to be installed in a custom directory,

SAGE_LOCAL cannot be the installation directory *and* an implementation detail.

The current reality is that $SAGE_LOCAL is the installation directory and an implementation detail. So I am really not changing that.

comment:5 in reply to: ↑ 4 Changed 3 years ago by felixs

Replying to jdemeyer:

The current reality

you put "Allow SAGE_LOCAL to be customized" into the title. this (to me) implies that SAGE_LOCAL would become a user controlled variable (see above). i hope not! but is it really your intent?

i figured, that maybe/actually/hopefully you want to introduce prefix (defaulting to $(builddir)/local), then set SAGE_LOCAL=prefix. then do some modifications so the rest of dist can cope with it.

about the title... what about "implement pretend-prefix using SAGE_LOCAL" or "use SAGE_LOCAL to implement pretend-prefix"? (yes, "pretend", because it will look like prefix to the user, while the build process will still write to it).

comment:6 follow-up: Changed 3 years ago by mkoeppe

Jeroen, are you working on this? Which changes should go into this ticket, and which changes into #21479?

comment:7 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to mkoeppe:

Jeroen, are you working on this?

Yes.

Which changes should go into this ticket, and which changes into #21479?

My idea is that this ticket simply allows SAGE_LOCAL to be set to a different directory and that #21479 implements the ./configure logic to automatically set SAGE_LOCAL to the --prefix given.

comment:8 follow-up: Changed 3 years ago by mkoeppe

OK, great. I assume that you can test #21501 separately by just setting an environment variable, so #21501 should be a prereq of #21479, correct?

comment:9 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

Replying to mkoeppe:

OK, great. I assume that you can test #21501 separately by just setting an environment variable, so #21501 should be a prereq of #21479, correct?

Yes.

comment:10 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/21501

comment:11 Changed 3 years ago by jdemeyer

  • Commit set to 64e697ddb971366afb7382d429729effa260d4a0
  • Status changed from new to needs_review

New commits:

64e697dAllow a custom $SAGE_LOCAL directory

comment:12 follow-up: Changed 3 years ago by mkoeppe

  • build/pkgs/autotools/spkg-write-makefile

    a b source version-list 
    4545# Extract upstream autotools tarball
    4646cd "$SAGE_ROOT"
    4747PKG=autotools-`cat build/pkgs/autotools/package-version.txt`
    48 mkdir -p local/var/tmp/sage
    49 cd "$SAGE_ROOT/local/var/tmp/sage"
     48mkdir -p "$SAGE_LOCAL/tmp/sage"
     49cd "$SAGE_LOCAL/tmp/sage"
    5050tar xjf "$SAGE_ROOT/upstream/$PKG.tar.bz2"
    5151cd $PKG

Was dropping "var" intentional?

comment:13 in reply to: ↑ 12 Changed 3 years ago by jdemeyer

Replying to mkoeppe:

Was dropping "var" intentional?

Yes, it was. This is a really temporary directory. So it should be tmp, not var/tmp.

comment:14 Changed 3 years ago by mkoeppe

It seems a bit unusual, I don't think there's such a thing as a tmp directory other than /tmp or more precisely, whatever mktemp(1) provides.

But I don't insist. It's just a maintenance script for autotools, after all.

comment:15 Changed 3 years ago by jdemeyer

In any case, I have installed Sage with this patch to a custom $SAGE_LOCAL directory and all doctests pass. I also tested the command line, SageNB and the Jupyter notebook.

comment:16 Changed 3 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:17 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/21501 to 64e697ddb971366afb7382d429729effa260d4a0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 3 years ago by fbissey

  • Commit 64e697ddb971366afb7382d429729effa260d4a0 deleted

Interesting and I see Volker already merged it. Guys, I am expecting the link src/doc/common/themes/sage/static/thebe.js which is pointing to ../../../../../../local/share/thebe/thebe.js to be dangling for you. I have no solution for you, I usually deal with those at the package manager level. Although in that particular case I just added thebe.js to the source in place.

comment:19 Changed 3 years ago by mkoeppe

Oh, there's a symbolic link in our sources?

comment:20 Changed 3 years ago by mkoeppe

I've created #21527 for this and other cleanup issues for this patch.

comment:21 Changed 3 years ago by jdemeyer

This didn't exist when this ticket was created:

$ ls src/doc/common/themes/sage/static/thebe.js
ls: cannot access src/doc/common/themes/sage/static/thebe.js: No such file or directory
Note: See TracTickets for help on using tickets.