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:  sage8.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 )
This used to be a convenient way to forcerebuild the whole Sage library:
$ ./sage f sagelib ... Error: package 'sagelib' not found Note: if it is an oldstyle package, use p instead of i to install it
Similar for ./sage f doc
.
Change History (30)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 23 months ago by
comment:3 followup: ↓ 4 Changed 23 months ago by
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
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 followup: ↓ 6 Changed 23 months ago by
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
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 oldstyle package, use p instead of i to install it" exit 1 fi
comment:7 Changed 23 months ago by
 Branch set to u/jdemeyer/__sage__f_sagelib_no_longer_works
comment:8 Changed 23 months ago by
 Commit set to 98db16e5ef2bde43c5b6b2eb9dad030a18640559
 Status changed from new to needs_review
New commits:
98db16e  Remove Makefile target check

comment:9 Changed 23 months ago by
 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
 Status changed from positive_review to needs_work
Actually, upon testing this I take it back. This is rather userhostile 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
 Branch changed from u/jdemeyer/__sage__f_sagelib_no_longer_works to u/embray/build/sageivalidtargets
 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 addedthough I don't think every makefile target is necessarily useful or valid in this context.
comment:12 Changed 23 months ago by
 Commit set to a72231b28c624845f886381ceb8e001f640ed396
Branch pushed to git repo; I updated commit sha1. New commits:
a72231b  Have 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
 Commit changed from a72231b28c624845f886381ceb8e001f640ed396 to 5a0e5585f6287608b8d2f5246fa340f0cdd06223
Branch pushed to git repo; I updated commit sha1. New commits:
5a0e558  Some minor tweaksin particular package names must be spelled exactly

comment:14 Changed 23 months ago by
 Branch changed from u/embray/build/sageivalidtargets 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
 Branch changed from u/jdemeyer/__sage__f_sagelib_no_longer_works to u/embray/build/sageivalidtargets
 Commit changed from 98db16e5ef2bde43c5b6b2eb9dad030a18640559 to 5a0e5585f6287608b8d2f5246fa340f0cdd06223
comment:16 Changed 23 months ago by
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
No problem I've done that too.
comment:18 followup: ↓ 19 Changed 23 months ago by
needs review?
comment:19 in reply to: ↑ 18 Changed 23 months ago by
 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
 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
Apart from that, this works fine.
comment:22 Changed 23 months ago by
 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
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
 Status changed from needs_review to needs_work
comment:25 Changed 23 months ago by
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
 Commit changed from 5a0e5585f6287608b8d2f5246fa340f0cdd06223 to 2ad88aefbdf0b048768bb9554cd064d29d231aa0
Branch pushed to git repo; I updated commit sha1. New commits:
2ad88ae  this 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
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
 Status changed from needs_work to needs_review
comment:29 Changed 23 months ago by
 Status changed from needs_review to positive_review
comment:30 Changed 23 months ago by
 Branch changed from u/embray/build/sageivalidtargets to 2ad88aefbdf0b048768bb9554cd064d29d231aa0
 Resolution set to fixed
 Status changed from positive_review to closed
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.