Opened 19 months ago

Closed 15 months ago

Last modified 15 months ago

#25857 closed defect (fixed)

Toolchain dependencies that have circular self-dependencies should not be uninstalled before reinstalling/upgrading them

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.5
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: fb51d4e (Commits) Commit: fb51d4ef640bb74c3f8e09eb2c89da74fcc0e978
Dependencies: Stopgaps:

Description

On Cygwin I tried doing ./sage -f mpir and everything broke.

This is because my C compiler uses mpc and mpfr, and on Cygwin (see #25816) at least, it ends up using Sage's mpc and mpfr DLLs. This may be less of a problem on other platforms but I haven't given much thought to it yet.

The mpc and mpfr DLLs in Sage break when Sage's mpir gets uninstalled, so it's necessary at a minimum to also uninstall mpc and mpfr. But at that point it might also make sense to recursively uninstall anything that depends on mpir. This seems reasonable given that they'll have to be re-built anyways.

Change History (73)

comment:1 in reply to: ↑ description ; follow-up: Changed 19 months ago by jdemeyer

  • Priority changed from major to blocker

Replying to embray:

On Cygwin I tried doing ./sage -f mpir and everything broke.

Can you give more details :-)

I disagree with your solution: it just needs to reorder the operations from

  1. Uninstall
  2. Build
  3. Install

to

  1. Build
  2. Uninstall
  3. Install

That shouldn't break anything.

If this is not easy to fix, maybe we should just disable the uninstall feature for now?

comment:2 in reply to: ↑ 1 Changed 19 months ago by embray

Replying to jdemeyer:

Replying to embray:

On Cygwin I tried doing ./sage -f mpir and everything broke.

Can you give more details :-)

I think I did in the description. By "everything" I mean that as soon as mpir's configure tries to detect a C compiler, gcc breaks because it's loading DLLs from $SAGE_LOCAL (ugh), and those DLLs are in turn compiled against the mpir from Sage, and are incompatible with my system's GMP.

The problem here is my system's gcc using libraries from Sage, when it really shouldn't. But this is a little difficult to avoid, unless I can patch things somewhere so that executables from the system such as gcc or git are explicitly not run with Sage's environment modifications (e.g. to $PATH). I believe this is less of a problem on Linux.

I disagree with your solution: it just needs to reorder the operations from

Why? I think it's a rather sensible solution.

  1. Uninstall
  2. Build
  3. Install

to

  1. Build
  2. Uninstall
  3. Install

It sort of depends on what you mean by "order of operations". Currently, sage-spkg does the second process, which is certainly my preference. (This process has its own problems, e.g. when building Python--this is partly Python's fault, and partly our fault for not providing better isolation between build and runtime environments.)

However, when you run ./sage -f like I did, it manually calls make <pkg>-clean before calling make <pkg>. Perhaps it shouldn't.

I don't consider this a blocker though, unless the problem can be reproduced other than on Cygwin, since I'm pretty much the only person who does development on Cygwin, and this isn't a normal use case.

comment:3 follow-up: Changed 19 months ago by embray

FWIW I did lots of testing of the uninstaller on Linux, including doing odd things like ./sage -f mpir (and I did specifically focus on mpir due to its involvement in tricky dependency cycles). Never saw any problem like this.

comment:4 follow-up: Changed 19 months ago by embray

Anyways, a lot more packages have problems if you don't remove their previous versions before building new versions of those packages, than the other way around. In this case, it's because mpir has dependents that are in turn dependencies of the compiler. In that case I would just uninstall all of mpir's dependents as well, hence my suggestion of recursive uninstall.

I don't like uninstalling things before their replacements are built, but it seems that's pretty necessary a lot of the time. For that reason I think I'll place a higher priority on #25202.

comment:5 follow-up: Changed 19 months ago by embray

This reminds me--did you ever get anywhere with creating a separate build-time package for mpir? Something like that might also help. Same for the other "toolchain-deps" (zlib, mpfr, mpc). They all share the problem that breaking them can break the C compiler which, in turn, needs them to build.

comment:6 in reply to: ↑ 4 Changed 19 months ago by jdemeyer

Replying to embray:

In that case I would just uninstall all of mpir's dependents as well, hence my suggestion of recursive uninstall.

How would you deal with a Sage-built GCC? That will obviously depend on Sage's MPIR too.

comment:7 in reply to: ↑ 5 Changed 19 months ago by jdemeyer

Replying to embray:

This reminds me--did you ever get anywhere with creating a separate build-time package for mpir?

Creating a separate package might simplify the build system, but it wouldn't fix the uninstall problem.

comment:8 in reply to: ↑ 3 ; follow-up: Changed 19 months ago by jdemeyer

Replying to embray:

FWIW I did lots of testing of the uninstaller on Linux, including doing odd things like ./sage -f mpir (and I did specifically focus on mpir due to its involvement in tricky dependency cycles). Never saw any problem like this.

Did you do that with a Sage-built GCC? That's where things get tricky.

comment:9 Changed 19 months ago by embray

A few additional thoughts:

a) As for sage-gcc, recursively uninstalling all dependents doubly makes sense. If you rebuild mpir you have to rebuild gcc anyways.

b) I'm thinking at the very least recursive uninstall should happen for all the "toolchain-deps"

c) Something I think would make all of this work better (either instead of, or parallel to separate packages), would be to have a separate install prefix, either under $SAGE_LOCAL or parallel to $SAGE_LOCAL or all toolchain dependencies. This prefix would be superseded by the usual SAGE_LOCAL paths, so the bootstrap toolchain never entirely goes away, but packages that are otherwise dependents of the toolchain can still be upgraded without breaking the existing toolchain.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 19 months ago by embray

Replying to jdemeyer:

Replying to embray:

FWIW I did lots of testing of the uninstaller on Linux, including doing odd things like ./sage -f mpir (and I did specifically focus on mpir due to its involvement in tricky dependency cycles). Never saw any problem like this.

Did you do that with a Sage-built GCC? That's where things get tricky.

I did, and I don't remember having any problems (again, on Linux), but that was a long time ago now so my memory is hazy.

comment:11 in reply to: ↑ 10 Changed 19 months ago by jdemeyer

Replying to embray:

I did, and I don't remember having any problems (again, on Linux)

Maybe your system GMP libraries were compatible with the Sage MPIR libraries, so the GCC-in-Sage used your system GMP library while MPIR was being rebuilt? Just guessing...

comment:12 Changed 19 months ago by embray

I'd think it would almost have to. I don't see how else it could work.

All the more reason to have separate copies as "toolchain dependencies", and even then only needed when doing silly things like building gcc. Then, a developer wanting to do things (such as I was doing) like experiment with patching and reconfiguring mpir can do so on top of that without breaking the toolchain.

comment:13 Changed 19 months ago by jdemeyer

The question is what can we do right now to fix this serious bug?

comment:14 follow-up: Changed 19 months ago by embray

Unless someone can reproduce this outside of Cygwin, I don't think it's that serious. I just installed sage-gcc on a Linux build and am trying all manner of ./sage -f mpir/gcc/etc without any problems.

I'm going to keep thinking about it though. If I can find a simple solution that works for Cygwin I believe it would work on Linux as well. Perhaps one very localized solution would be that if one of the toolchain-deps gets uninstalled they should all be uninstalled and rebuilt.

comment:15 in reply to: ↑ 14 Changed 18 months ago by jdemeyer

Replying to embray:

Unless someone can reproduce this outside of Cygwin, I don't think it's that serious.

Given the analysis, I don't see any reason why this would be limited to Cygwin.

comment:16 Changed 18 months ago by jdemeyer

Just reproduced it on Ubuntu 16.04.1 LTS (CPU architecture ppc64le) with system compiler

gcc (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

When running ./sage -f mpir on Sage 8.3.rc1, the problem is as I predicted:

$ ldd local/libexec/gcc/powerpc64le-unknown-linux-gnu/7.2.0/cc1
        linux-vdso64.so.1 =>  (0x00003fffa2290000)
        libmpc.so.3 => /home/jdemeyer/sage-test/local/lib/libmpc.so.3 (0x00003fffa2260000)
        libmpfr.so.6 => /home/jdemeyer/sage-test/local/lib/libmpfr.so.6 (0x00003fffa21c0000)
        libgmp.so.23 => not found
        libdl.so.2 => /lib/powerpc64le-linux-gnu/libdl.so.2 (0x00003fffa2180000)
        libz.so.1 => /home/jdemeyer/sage-test/local/lib/libz.so.1 (0x00003fffa2140000)
        libm.so.6 => /lib/powerpc64le-linux-gnu/libm.so.6 (0x00003fffa2050000)
        libc.so.6 => /lib/powerpc64le-linux-gnu/libc.so.6 (0x00003fffa1e80000)
        /lib64/ld64.so.2 (0x00003fffa22b0000)
        libgmp.so.23 => not found
        libgmp.so.23 => not found

causing obvious build failures.

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

comment:17 Changed 18 months ago by embray

I'll try to reproduce that on x86_64, as I don't have access to that architecture. But I see what you're saying in general.

There are two cases here:

  • without sage-gcc
  • with sage-gcc

The reason I focused here on Cygwin is that this a problem for Cygwin even without sage-gcc due to problems with the DLL search path on Windows. It non-sage-gcc should not have any problem on Linux.

But it is a problem certainly for sage-gcc if it's built against a sage-mpir that is not compatible somehow or other with the system's libgmp.

This all goes back to (one of) Sage's original sins, which is that you're building software packages in a build environment that is also the installation target. For most packages this is not a big problem, but for your build toolchain itself it certainly is. This is why I think that for bootstrapping purposes the build toochain and its dependencies should have some level of isolation from the rest of the install target.

As for a solution, I'd certainly be willing to disable the new-style uninstallation as the default behavior for now. Honestly, I would have preferred to wait until more/all of DESTDIR-related updates were in, as well as #25140, though be design it's supposed to be able to work without that (and does in most cases). But I'm hesitant because for the majority of packages it does work well now, and I'm not entirely sure what the impact would be of fully disabling it now (but maybe it would be small).

So I think for now it still might be best to find a middle-ground. What if, for now, for toolchain dependencies only, we ensure that their files are not removed before reinstalling them? This could work in part with cooperation from sage-spkg by adding a flag similar to pip's --ignore-installed which causes it to ignore dependencies that are already satisfied and just install on top of them, rather than uninstalling conflicting/existing dependencies first.

comment:18 Changed 18 months ago by jdemeyer

  • Milestone changed from sage-8.4 to sage-8.3

Personally, I would prefer to revert uninstall completely because that's the safest operation. But if you have a different solution which is reasonably simple, that might be OK for me too.

comment:19 Changed 18 months ago by embray

I mean, this issue needs to be resolved somehow anyways--the new package uninstallation is not going away long-term.

comment:20 Changed 18 months ago by embray

I'm currently testing a fix that I think is pretty uninvasive. I would still consider it a work-around but it's reasonable.

comment:21 Changed 18 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-25857
  • Commit set to 99760919b4cf66c55dba78d7a4153273b474948d
  • Status changed from new to needs_review
  • Summary changed from Uninstall should probably be recursive to Toolchain dependencies that have circular self-dependencies should not be uninstalled before reinstalling/upgrading them

New commits:

2965003add vim modeline for this file
f5f68daAdd a package that by all rights should be included in this list
15c7b88move this list into a TOOLCHAIN_DEPS variable we can use to inspect this list elsewhere
1954e5eAdd -k/--keep-existing to sage-spkg
f1411c5From <spkg>-clean targets in the Makefile, just remove the package's
9976091Use sage-spkg --keep-existing when installing/re-installing packages in

comment:22 Changed 18 months ago by embray

  • Status changed from needs_review to needs_work

f1411c5 works for toolchain packages, but can actually break other packages that rely on the new-style uninstaller to remove files should should be removed in order to reinstall that package. This came up for me in the case of elliptic_curves. Simply deleting the stamp file can be a problem, because it means erasing the file manifest for that package (perhaps an argument that they should not be one in the same, but that's another issue).

This can be fixed by further narrowing things so that <spkg>-clean works the old way only for packages in TOOLCHAIN_DEPS.

comment:23 Changed 18 months ago by git

  • Commit changed from 99760919b4cf66c55dba78d7a4153273b474948d to 22a1f34f0157cc5f9599e094e0e23da040ef768f

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

22a1f34Revert to using sage-spkg-uninstall in <spkg>-clean targets, but add a

comment:24 Changed 18 months ago by embray

  • Status changed from needs_work to needs_review

comment:25 follow-up: Changed 18 months ago by vbraun

IMHO this would be a mistake to merge at the last minute; This does not fix any bug that generates wrong answers, has no testsuite coverage, etc. Its much safer to merge it at the beginnig of the next beta phase...

comment:26 follow-up: Changed 18 months ago by jdemeyer

I don't like the change $(inst_zlib) -> zlib. It will prevent using a system zlib if we ever want that.

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

comment:27 Changed 18 months ago by vbraun

  • Priority changed from blocker to major

comment:28 Changed 18 months ago by jdemeyer

  • Priority changed from major to blocker

This fixes a very serious bug when building from source. You may not like the solution, but the bug is real.

comment:29 follow-ups: Changed 18 months ago by vbraun

It doesn't break configure && make as far as I can tell. Basically there is an obscure makefile target that requires a full rebuild to recover. How is that a release blocker, just don't do it until its fixed.

In a similar vein, make doc frequently breaks incremental builds and nobody thinks thats a release blocker.

Last edited 18 months ago by vbraun (previous) (diff)

comment:30 in reply to: ↑ 26 Changed 18 months ago by embray

Replying to jdemeyer:

I don't like the change $(inst_zlib) -> zlib. It will prevent using a system zlib if we ever want that.

I'm not sure that's true. My system for enabling use of more system packages wouldn't be impacted by that, I don't think. But if that's your only review comment I'll fix it; it's not a big deal (I think it's still useful to have a variable listing the bare package names, but I can add a separate one).

comment:31 in reply to: ↑ 29 Changed 18 months ago by embray

Replying to vbraun:

It doesn't break configure && make as far as I can tell. Basically there is an obscure makefile target that requires a full rebuild to recover. How is that a release blocker, just don't do it until its fixed.

In a similar vein, make doc frequently breaks incremental builds and nobody thinks thats a release blocker.

I actually kind of agree with Volker on this one :) But I'm also sympathetic to it since:

a) I broke it.

b) I was bitten by this issue myself, and on Cygwin it can rather badly break patchbots since it affects Cygwin even without installing sage-gcc.

c) The fix is simple (IMO) and if it helps Jeroen sleep at night I don't see the problem.

Last edited 18 months ago by embray (previous) (diff)

comment:32 in reply to: ↑ 25 Changed 18 months ago by embray

Replying to vbraun:

IMHO this would be a mistake to merge at the last minute; This does not fix any bug that generates wrong answers, has no testsuite coverage, etc. Its much safer to merge it at the beginnig of the next beta phase...

Also it wouldn't be "last minute" it if we had a release schedule that created fewer artificial "panic windows".

comment:33 follow-up: Changed 18 months ago by jdemeyer

I'm not very comfortable with code like

	+$(AM_V_at)sage-logger -p '$$(SAGE_SPKG) \
		$(if $(filter $(1),$(TOOLCHAIN_DEPS)),--keep-existing) \
		$(1)-$(2)' '$$(SAGE_LOGS)/$(1)-$(2).log'

I never liked your idea about defining these kind of rules in Makefile syntax. How am I supposed to understand what is going on here?

comment:34 follow-up: Changed 18 months ago by jdemeyer

This comment

# Note: This list is determined from the dependencies of the TOOLCHAIN
# packages which include gcc, and optionally ccache; in principle this
# list is redundant.

is not completely accurate since it should also include the dependencies of the system toolchain, which may use Sage libraries (or did we fix that?)

comment:35 follow-up: Changed 18 months ago by jdemeyer

Also, TOOLCHAIN_DEPS should only contain the runtime dependencies of the toolchain (I mean the runtime of the toolchain, when building packages), so not xz.

comment:36 follow-up: Changed 18 months ago by jdemeyer

Setting this to needs_work for me because of 26 and 34 and 35 but I haven't finished testing.

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

comment:37 Changed 18 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:38 in reply to: ↑ 33 ; follow-up: Changed 18 months ago by embray

Replying to jdemeyer:

I'm not very comfortable with code like

	+$(AM_V_at)sage-logger -p '$$(SAGE_SPKG) \
		$(if $(filter $(1),$(TOOLCHAIN_DEPS)),--keep-existing) \
		$(1)-$(2)' '$$(SAGE_LOGS)/$(1)-$(2).log'

I never liked your idea about defining these kind of rules in Makefile syntax. How am I supposed to understand what is going on here?

I'm sorry but you'll have to live with that. Read the docs? With GNU make functions, if you ignore the $ (or at least, understand that the function call is treated the same as any other variable expansion), and the commas, then it reads more or less like LISP:

(if (filter <the-dependency> TOOLCHAIN_DEPS) --keep-existing)

where filter expands to <the-dependency> if it is found in TOOLCHAIN_DEPS, and if expands to its second argument if its first argument is non-empty (or to its optional third argument if the first argument is empty).

Last edited 18 months ago by embray (previous) (diff)

comment:39 in reply to: ↑ 34 ; follow-up: Changed 18 months ago by embray

Replying to jdemeyer:

This comment

# Note: This list is determined from the dependencies of the TOOLCHAIN
# packages which include gcc, and optionally ccache; in principle this
# list is redundant.

is not completely accurate since it should also include the dependencies of the system toolchain, which may use Sage libraries (or did we fix that?)

Why would the system toolchain ever use Sage libraries unless the user were doing something like setting LD_LIBRARY_PATH?

comment:40 in reply to: ↑ 35 ; follow-up: Changed 18 months ago by embray

Replying to jdemeyer:

Also, TOOLCHAIN_DEPS should only contain the runtime dependencies of the toolchain (I mean the runtime of the toolchain, when building packages), so not xz.

I'm not really clear on where xz fits into this. Why is it listed as a dependency of gcc in the first place?

comment:41 in reply to: ↑ 40 ; follow-up: Changed 18 months ago by jdemeyer

Replying to embray:

Why is it listed as a dependency of gcc in the first place?

Only because the GCC tarball is xz-compressed.

comment:42 in reply to: ↑ 39 ; follow-up: Changed 18 months ago by jdemeyer

Replying to embray:

Why would the system toolchain ever use Sage libraries unless the user were doing something like setting LD_LIBRARY_PATH?

We certainly did that in the past but maybe not anymore.

comment:43 in reply to: ↑ 42 Changed 18 months ago by embray

Replying to jdemeyer:

Replying to embray:

Why would the system toolchain ever use Sage libraries unless the user were doing something like setting LD_LIBRARY_PATH?

We certainly did that in the past but maybe not anymore.

Only on Cygwin, where it's required for dlopen to work, but it has no other effect on Cygwin. There's no reason otherwise to set LD_LIBRARY_PATH in the Sage environment in general since we mostly rely on RPATH. There are just a few individual special cases where it's set (but not for running the compiler).

comment:44 in reply to: ↑ 41 Changed 18 months ago by embray

Replying to jdemeyer:

Replying to embray:

Why is it listed as a dependency of gcc in the first place?

Only because the GCC tarball is xz-compressed.

Wow, ok...

I'll just, drop the commit that added it then.

Last edited 18 months ago by embray (previous) (diff)

comment:45 in reply to: ↑ 29 ; follow-up: Changed 18 months ago by jdemeyer

Replying to vbraun:

It doesn't break configure && make as far as I can tell.

It doesn't break a build from scratch but I don't understand why. It should break since mpir is built twice (once before GCC and once after GCC).

comment:46 follow-up: Changed 18 months ago by jdemeyer

When building mpir the second time (after GCC) I get

No record that 'mpir' was ever installed; skipping uninstall

Can anybody explain this?

comment:47 in reply to: ↑ 45 Changed 18 months ago by embray

Replying to jdemeyer:

Replying to vbraun:

It doesn't break configure && make as far as I can tell.

It doesn't break a build from scratch but I don't understand why. It should break since mpir is built twice (once before GCC and once after GCC).

How exactly are you reproducing the issue? I can reproduce it by running ./sage -f mpir. The reason for this is very specific to ./sage -f, because it explicitly calls make clean-mpir before trying to rebuild mpir. During a normal build from scratch that does not happen.

comment:48 in reply to: ↑ 46 Changed 18 months ago by embray

Replying to jdemeyer:

When building mpir the second time (after GCC) I get

No record that 'mpir' was ever installed; skipping uninstall

Can anybody explain this?

Are you getting that with this patch, or on bare 8.3.rc1.

Either way that's normal. That's just what sage-spkg-uninstall says if it can't find local/var/lib/sage/installed/mpir-<whatever>. As far is it knows there's nothing to uninstall.

comment:49 Changed 18 months ago by git

  • Commit changed from 22a1f34f0157cc5f9599e094e0e23da040ef768f to 3cb9be71e5aa428dc32c8b6d8ba578444536d343

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

2965003add vim modeline for this file
9c1b4f6move this list into a TOOLCHAIN_DEPS variable we can use to inspect this list elsewhere
311989dAdd -k/--keep-existing to sage-spkg
89205feFrom <spkg>-clean targets in the Makefile, just remove the package's
cdc6d70Use sage-spkg --keep-existing when installing/re-installing packages in
0a2ab6aRevert to using sage-spkg-uninstall in <spkg>-clean targets, but add a
3cb9be7make the toolchain dependency stamp files directly

comment:50 Changed 18 months ago by git

  • Commit changed from 3cb9be71e5aa428dc32c8b6d8ba578444536d343 to 2da7f8e7ba0fd588bf3cff48b4c78798288e1f73

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

2da7f8eupdated questionable comment; maybe a little better?

comment:51 in reply to: ↑ 36 Changed 18 months ago by embray

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Setting this to needs_work for me because of 26 and 34 and 35 but I haven't finished testing.

I think I've addressed all of these:

[x] removed xz from TOOLCHAIN_DEPS
[x] used $(MAKE) $(inst_pkg) for each package in TOOLCHAIN_DEPS (serially)
[x] updated the comment; though I don't know if what it says now is really any better (I think it is though)

Sorry for my flippancy about make syntax constructs but I really think it makes things simpler; there's nothing out of the ordinary about it.

Last edited 18 months ago by embray (previous) (diff)

comment:52 Changed 18 months ago by jdemeyer

  • Milestone changed from sage-8.3 to sage-8.4
  • Priority changed from blocker to critical

I changed my mind about the urgency if it only happens on force rebuilds (sage -f ...)

comment:53 Changed 18 months ago by embray

FWIW it can also screw up patchbots since they're regularly jumping back and forth between branches (which might end up requiring a rebuild of mpir and/or dependencies).

But if that's still fixed by the next beta release I don't see it as that big a deal.

Last edited 18 months ago by embray (previous) (diff)

comment:54 in reply to: ↑ 38 ; follow-up: Changed 18 months ago by jdemeyer

Replying to embray:

I'm sorry but you'll have to live with that. Read the docs?

Understanding this syntactically is not the same as actually understanding it semantically. Whenever I try to review this ticket, I also end up realizing that there is no way to be sure that this Makefile syntax does what it is supposed to do.

And your suggestion to run make -f build/make/Makefile -n DEBUG_RULES=1 would be useful if it would actually do what it said: the toolchain-deps rules that you change on this ticket do not appear there.

I'm not making this comment just to be annoying or pedantic. I really do think that it's a serious problem with #21524.

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

comment:55 follow-up: Changed 18 months ago by jdemeyer

Some other comments:

  1. the flags SAGE_INSTALL_FETCH_ONLY, SAGE_CHECK and SAGE_KEEP_BUILT_SPKGS are meant to be usable as environment variables. So I would do the same for KEEP_EXISTING (rename it to SAGE_KEEP_EXISTING and remove KEEP_EXISTING=0).
  1. keep the whitespace line in the -clean rule.

comment:56 in reply to: ↑ 55 ; follow-up: Changed 18 months ago by embray

Replying to jdemeyer:

Some other comments:

  1. the flags SAGE_INSTALL_FETCH_ONLY, SAGE_CHECK and SAGE_KEEP_BUILT_SPKGS are meant to be usable as environment variables. So I would do the same for KEEP_EXISTING (rename it to SAGE_KEEP_EXISTING and remove KEEP_EXISTING=0).

Ok.

  1. keep the whitespace line in the -clean rule.

Yes, weirdly I thought I already fixed that. I don't know why the fix went away again.

comment:57 in reply to: ↑ 54 ; follow-up: Changed 18 months ago by embray

Replying to jdemeyer:

Replying to embray:

I'm sorry but you'll have to live with that. Read the docs?

Understanding this syntactically is not the same as actually understanding it semantically. Whenever I try to review this ticket, I also end up realizing that there is no way to be sure that this Makefile syntax does what it is supposed to do.

And your suggestion to run make -f build/make/Makefile -n DEBUG_RULES=1 would be useful if it would actually do what it said: the toolchain-deps rules that you change on this ticket do not appear there.

That's just for printing the automatically generated rules, not for printing every rule in the makefile, not statically-defined targets, like toolchain-deps.

I'm not sure what's unclear about that target.

I'm not making this comment just to be annoying or pedantic. I really do think that it's a serious problem with #21524.

I believe your intentions are sincere, but I still find this objection very confusing. I feel like you're just stubbornly refusing to learn for some reason, because I find it easy to understand, and to check; maybe just not as easy as, say, a Python script. I don't know if that's what's going on; that's just what it feels like to me. Maybe a tutorial on make would help; I don't know.

Anyways, I think that discussion is outside the scope of this ticket.

Last edited 18 months ago by embray (previous) (diff)

comment:58 Changed 18 months ago by embray

  • Status changed from needs_review to needs_work

comment:59 in reply to: ↑ 56 ; follow-up: Changed 18 months ago by embray

Replying to embray:

Replying to jdemeyer:

Some other comments:

  1. the flags SAGE_INSTALL_FETCH_ONLY, SAGE_CHECK and SAGE_KEEP_BUILT_SPKGS are meant to be usable as environment variables. So I would do the same for KEEP_EXISTING (rename it to SAGE_KEEP_EXISTING and remove KEEP_EXISTING=0).

Ok.

Actually, the more I think about it, I think we should avoid adding yet another environment variable for now. Is this something we actually need? If there's not an obvious use case for setting this flag for all packages I think it should be avoided. The name SAGE_KEEP_EXISTING is also very unclear as to what it does. Keep existing what? In the context of the sage-spkg script it makes sense, but it's very unobvious otherwise.

comment:60 Changed 18 months ago by git

  • Commit changed from 2da7f8e7ba0fd588bf3cff48b4c78798288e1f73 to 85a368f2b4763f4721fb6541953573805cb4b14f

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

85a368feach of these templates should have an empty newline at the end to make them more readable when printed one after the other

comment:61 Changed 18 months ago by embray

  • Status changed from needs_work to needs_review

comment:62 in reply to: ↑ 57 ; follow-up: Changed 18 months ago by embray

Replying to embray:

Replying to jdemeyer:

Replying to embray:

I'm sorry but you'll have to live with that. Read the docs?

Understanding this syntactically is not the same as actually understanding it semantically. Whenever I try to review this ticket, I also end up realizing that there is no way to be sure that this Makefile syntax does what it is supposed to do.

Which syntax specifically? What are you trying to check?

Last edited 18 months ago by embray (previous) (diff)

comment:63 in reply to: ↑ 59 ; follow-up: Changed 18 months ago by jdemeyer

Replying to embray:

If there's not an obvious use case for setting this flag for all packages I think it should be avoided.

Why? I would argue the opposite: it doesn't harm to support it as an environment variable, so why should we not do that?

The name SAGE_KEEP_EXISTING is also very unclear as to what it does.

Then call it SAGE_SKIP_UNINSTALL or whatever.

comment:64 in reply to: ↑ 62 ; follow-up: Changed 18 months ago by jdemeyer

Replying to embray:

Which syntax specifically? What are you trying to check?

I'm just trying to check that this branch does what it seems to be doing. It looks like it does the right thing (skipping uninstall for toolchain packages) but how I can be sure?

By now, I've looked at this ticket a few times and I guess it's OK. I'll wait for your reply on the environment variable issue before doing a final review.

comment:65 in reply to: ↑ 64 Changed 18 months ago by embray

Replying to jdemeyer:

Replying to embray:

Which syntax specifically? What are you trying to check?

I'm just trying to check that this branch does what it seems to be doing. It looks like it does the right thing (skipping uninstall for toolchain packages) but how I can be sure?

I'm still not sure what you're asking here. Are you just asking how to test code in a makefile? How can you be sure any code does what you think it looks like it does? I'm not trying to be facetious--I'm just trying to get to the bottom of what your concern is.

comment:66 in reply to: ↑ 63 Changed 18 months ago by embray

Replying to jdemeyer:

Replying to embray:

If there's not an obvious use case for setting this flag for all packages I think it should be avoided.

Why? I would argue the opposite: it doesn't harm to support it as an environment variable, so why should we not do that?

I don't agree. Adding any new "feature" means you have to support it. The main purpose of this change (although it does add new features in the form of new command-line flags to some scripts) was not really to add new features so much as to workaround a specific problem. So better to keep the feature surface area of the change minimal. I also believe the YAGNI principle applies here.

But if you really want it for some reason a compromise might be to change the name but not document it anywhere. I'm also a little unsure about this in that it's really a behavior of the uninstallation that this is changing, not the installation. If anything it should be an environment variable read by the uninstaller, but there again it adds needless complication... (That last sentence didn't make sense; I misremembered what my own patch does :)

Last edited 18 months ago by embray (previous) (diff)

comment:67 Changed 18 months ago by git

  • Commit changed from 85a368f2b4763f4721fb6541953573805cb4b14f to fb51d4ef640bb74c3f8e09eb2c89da74fcc0e978

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

fb51d4eallow 'sage-spkg -k' to also be implied by setting the environment varible SAGE_SPKG_SKIP_UNINSTALL=yes

comment:68 Changed 18 months ago by embray

How about this? I called it SAGE_SPKG_SKIP_UNINSTALL and noted it in the documentation at the top of sage-spkg, but not elsewhere, at least for now.

I'd like to get this done already; I think this fix is too important to be held up any longer by bikeshedding.

comment:69 Changed 17 months ago by dimpase

Perhaps this should get #25188 as a dependence, so that the latter and this are merged together, with just one new configure tarball instead of two...

comment:70 Changed 17 months ago by embray

Looks like the latest patchbot build failed due to #18438.

Dima, that's not a bad idea, but I'd like to see a positive review on both first.

comment:71 Changed 15 months ago by dimpase

  • Reviewers set to Dima Pasechnik, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:72 Changed 15 months ago by vbraun

  • Branch changed from u/embray/ticket-25857 to fb51d4ef640bb74c3f8e09eb2c89da74fcc0e978
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:73 Changed 15 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.