Opened 2 years ago

Closed 23 months ago

#25078 closed defect (fixed)

./sage -f sagelib no longer works

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.2
Component: build Keywords:
Cc: embray Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 2ad88ae (Commits) Commit: 2ad88aefbdf0b048768bb9554cd064d29d231aa0
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This used to be a convenient way to force-rebuild the whole Sage library:

$ ./sage -f sagelib
...
Error: package 'sagelib' not found
Note: if it is an old-style package, use -p instead of -i to install it

Similar for ./sage -f doc.

Change History (30)

comment:1 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 23 months ago by embray

This reminds me, it's been on my TODO list to just make "sagelib" another spkg (with some special disposition that allows its "source" to be found and built in a local directory rather than downloaded as a tarball and extracted to a temp dir). I'm not sure why this would be broken now though.

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

In src/bin/sage there's a kind of hackish "grep" of build/make/Makefile to ensure that the argument to ./sage -f (mostly assumed to be name of an SPKG) is actually a make target. But it seems from the usage you describe here, really ./sage -f should work for almost any make target, not just SPKGs.

I guess either the check could be made more generic or just removed entirely.

Also is this actually documented behavior? If not I wouldn't consider it a "blocker". The --help just says of -f "force build of the given Sage packages". It's not clear to me whether "sagelib" and "doc" are considered "Sage packages" (per my above comment maybe the should be...)

comment:4 in reply to: ↑ 3 Changed 23 months ago by jdemeyer

Replying to embray:

Also is this actually documented behavior?

I don't think so, but it should be. I have certainly recommended running ./sage -f sagelib to people.

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

Well how would you suggest handling it? IMO the actual behavior you desire should be documented before moving forward on fixing it. What targets should it work for? Just "sagelib" and "doc"? Or others as well? Previously, it technically worked for any target in the Makefile.

comment:6 in reply to: ↑ 5 Changed 23 months ago by jdemeyer

Replying to embray:

Previously, it technically worked for any target in the Makefile.

It is actually by design that ./sage -i works that way. So maybe the easiest fix is removing this check:

        # First check that $PKG is actually a Makefile target
        # Since https://trac.sagemath.org/ticket/21524 there is not explicitly
        # a target for the package, but if it has a vers_<pkgname> variable
        # there will also be a generated rule for it
        if ! grep "^vers_$PKG = " build/make/Makefile >/dev/null; then
            echo >&2 "Error: package '$PKG' not found"
            echo >&2 "Note: if it is an old-style package, use -p instead of -i to install it"
            exit 1
        fi

comment:7 Changed 23 months ago by jdemeyer

  • Branch set to u/jdemeyer/__sage__f_sagelib_no_longer_works

comment:8 Changed 23 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 98db16e5ef2bde43c5b6b2eb9dad030a18640559
  • Status changed from new to needs_review

New commits:

98db16eRemove Makefile target check

comment:9 Changed 23 months ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

FWIW I don't think 'build' commands should be combined into Sage's runtime interface at all. The sole exception might be sage -i (or an equivalent) for installing optional packages, and even that has its problems.

comment:10 Changed 23 months ago by embray

  • Status changed from positive_review to needs_work

Actually, upon testing this I take it back. This is rather user-hostile because if you run ./sage -i <unknown_or_misspelled_package> it will still just fire up make, possibly rebuild some other things (as it did for me, since I had to rebuild my toolchain packages due to previous tinkering) and only much later give an error from make that the target is not found (and a hard to understand error, at that, if you don't know what you're looking for).

comment:11 Changed 23 months ago by embray

  • Branch changed from u/jdemeyer/__sage__f_sagelib_no_longer_works to u/embray/build/sage-i-valid-targets
  • Commit 98db16e5ef2bde43c5b6b2eb9dad030a18640559 deleted

What about something like this? If you run make list it simply prints a list of all the packages the makefile knows about, as well as some additional "packages" listed in the SAGE_I_TARGETS variable. Ccurrently just "sagelib" and "doc" but others could be added--though I don't think every makefile target is necessarily useful or valid in this context.

comment:12 Changed 23 months ago by git

  • Commit set to a72231b28c624845f886381ceb8e001f640ed396

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

a72231bHave the makefile itself provide a list of the valid targets that can be used with sage -i, for example.

comment:13 Changed 23 months ago by git

  • Commit changed from a72231b28c624845f886381ceb8e001f640ed396 to 5a0e5585f6287608b8d2f5246fa340f0cdd06223

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

5a0e558Some minor tweaks--in particular package names must be spelled exactly

comment:14 Changed 23 months ago by dimpase

  • Branch changed from u/embray/build/sage-i-valid-targets to u/jdemeyer/__sage__f_sagelib_no_longer_works
  • Commit changed from 5a0e5585f6287608b8d2f5246fa340f0cdd06223 to 98db16e5ef2bde43c5b6b2eb9dad030a18640559

I agree with Erik that the misspelt package names are a nuisance, resulting in misleading logs.

comment:15 Changed 23 months ago by embray

  • Branch changed from u/jdemeyer/__sage__f_sagelib_no_longer_works to u/embray/build/sage-i-valid-targets
  • Commit changed from 98db16e5ef2bde43c5b6b2eb9dad030a18640559 to 5a0e5585f6287608b8d2f5246fa340f0cdd06223

I don't think Dima intended to change the branch...


New commits:

a72231bHave the makefile itself provide a list of the valid targets that can be used with sage -i, for example.
5a0e558Some minor tweaks--in particular package names must be spelled exactly

comment:16 Changed 23 months ago by gh-dimpase

apologies for messing up the branch. I forgot that this happens if the ticket was changed in the meantime, and I didn't click somthing (I am hopeless with GUIs in general :-))

comment:17 Changed 23 months ago by embray

No problem I've done that too.

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

needs review?

comment:19 in reply to: ↑ 18 Changed 23 months ago by embray

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

needs review?

I guess, if you agree with my general analysis of the situation. I posted my branch as an offer for an alternative solution, but it only "needs review" if you agree with my previous comments.

comment:20 Changed 23 months ago by jdemeyer

  • Authors changed from Jeroen Demeyer to Erik Bray
  • Reviewers changed from Erik Bray to Jeroen Demeyer
  • Status changed from needs_review to needs_work

What's the point of

@$(MAKE) -q build/make/Makefile >&2

Wouldn't it make more sense to actually build build/make/Makefile?

comment:21 Changed 23 months ago by jdemeyer

Apart from that, this works fine.

comment:22 Changed 23 months ago by embray

  • Status changed from needs_work to needs_review

I think you misread that line. That's exactly what it's doing.

comment:23 Changed 23 months ago by jdemeyer

I think you either misread the make manpage or you have a different version of make than me.

       -q, --question
            ``Question mode''.  Do not run any commands, or print anything; just return an exit status that  is  zero  if  the  specified  targets  are
            already up to date, nonzero otherwise.

comment:24 Changed 23 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:25 Changed 23 months ago by embray

Oops, sorry, you're right. I had it confused with -s for --silent (which confusingly can also be spelled --quiet, but not -q).

comment:26 Changed 23 months ago by git

  • Commit changed from 5a0e5585f6287608b8d2f5246fa340f0cdd06223 to 2ad88aefbdf0b048768bb9554cd064d29d231aa0

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

2ad88aethis was supposed to be -s, not -q; let's use long names instead so we don't confuse ourselves again

comment:27 Changed 23 months ago by embray

The overall intent was so that in the (unusual) case that build/make/Makefile has to be generated in order to produce the list, the only output to stdout would still be the list itself.

comment:28 Changed 23 months ago by embray

  • Status changed from needs_work to needs_review

comment:29 Changed 23 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:30 Changed 23 months ago by vbraun

  • Branch changed from u/embray/build/sage-i-valid-targets to 2ad88aefbdf0b048768bb9554cd064d29d231aa0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.