Opened 9 months ago

Last modified 7 days ago

#31163 positive_review enhancement

build/make/Makefile.in: For script packages without spkg-install, run "sage -info" and exit with error

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: build Keywords:
Cc: gh-kliem, jhpalmieri, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: u/mkoeppe/build_make_makefile_in__for_script_packages_without_spkg_install__run__sage__info__and_exit_with_error (Commits, GitHub, GitLab) Commit: b485d469cb1f61774b3f098b1e4277e66bd712fd
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

Then scripts like build/pkgs/perl_cpan_polymake_prereq/spkg-install can be removed.

This will help toward #31522 - because the GH Actions scripts can just avoid trying to install these packages.

Change History (25)

comment:1 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:2 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:3 Changed 10 days ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 10 days ago by mkoeppe

  • Summary changed from sage-spkg: For script packages without spkg-install, run "sage -info" and exit with error to build/make/Makefile.in: For script packages without spkg-install, run "sage -info" and exit with error

comment:5 Changed 10 days ago by mkoeppe

  • Branch set to u/mkoeppe/build_make_makefile_in__for_script_packages_without_spkg_install__run__sage__info__and_exit_with_error

comment:6 Changed 10 days ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc gh-kliem jhpalmieri slelievre added
  • Commit set to 8f782c01dbc947dcf386303444402b280346f14c
  • Status changed from new to needs_review

New commits:

2576f86build/make/Makefile.in: If a script package has no spkg-install, run "sage -info" and exit with error
feb8de7build/pkgs/: Remove spkg-install scripts for dummy script packages
8f782c0.github/workflows/tox-{optional,experimental}.yml: Do not try to test dummy script packages

comment:7 Changed 10 days ago by mkoeppe

  • Reviewers set to https://github.com/sagemath/sagetrac-mirror/actions/runs/1249185115

comment:8 Changed 9 days ago by git

  • Commit changed from 8f782c01dbc947dcf386303444402b280346f14c to a7b63520b3cc2daa5a7da2f6b7218f40211d92fc

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

4b292bebuild/pkgs/perl_mongodb/spkg-install: Remove
a7b6352build/bin/sage-spkg-info: Fix display of system packages

comment:9 Changed 9 days ago by mkoeppe

comment:10 Changed 8 days ago by jhpalmieri

A small issue: running ./sage -i graphviz prints this at the end:

The following package(s) may have failed to build (not necessarily
during this run of 'make graphviz'):

It is safe to delete any log files and build directories, but they
contain information that is helpful for debugging build problems.
WARNING: If you now run 'make' again, the build directory of the
same version of the package will, by default, be deleted. Set the
environment variable SAGE_KEEP_BUILT_SPKGS=yes to prevent this.

Should sage --info ... also write the output to a log file with an appropriate error so that it gets flagged in this message?

comment:11 Changed 8 days ago by mkoeppe

I don't think this would be very useful. The output already goes to logs/install.log

comment:12 follow-ups: Changed 7 days ago by jhpalmieri

Also, and I'm guessing this is beyond the scope of this ticket, but on my Mac I did brew install graphviz and then ./configure, and it said

configure:43560: result: graphviz-none:                               using system package; SPKG will not be installed

It would be nice if make graphviz said that it was already installed as a system package, rather than giving the same error message as if it weren't installed.

comment:13 Changed 7 days ago by git

  • Commit changed from a7b63520b3cc2daa5a7da2f6b7218f40211d92fc to e65b3092f97226196a5ea2de1b46508371745d00

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

e65b309bootstrap: Do not provide ./configure --enable-SPKG options for dummy optional packages

comment:14 Changed 7 days ago by git

  • Commit changed from e65b3092f97226196a5ea2de1b46508371745d00 to b485d469cb1f61774b3f098b1e4277e66bd712fd

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

b485d46m4/sage_spkg_collect.m4: Do not advertise dummy optional packages as installable

comment:15 in reply to: ↑ 12 Changed 7 days ago by mkoeppe

Replying to jhpalmieri:

Also, and I'm guessing this is beyond the scope of this ticket, but on my Mac I did brew install graphviz and then ./configure, and it said

result: graphviz-none:                               using system package; SPKG will not be installed

It also used to say:

perl_cpan_polymake_prereq-none:              no suitable system package; optional, use "./configure --enable-perl_cpan_polymake_prereq" to install

I just made some improvements to both of these messages. I am now getting:

pandoc-none:                                 using system package
...
perl_cpan_polymake_prereq-none:              no suitable system package; optional

Also I have removed the useless configure options --enable-perl_cpan_polymake_prereq (whose only function was to provoke an error at make time) -- so they are no longer shown in ./configure --help.

comment:16 in reply to: ↑ 12 Changed 7 days ago by mkoeppe

Replying to jhpalmieri:

It would be nice if make graphviz said that it was already installed as a system package, rather than giving the same error message as if it weren't installed.

This wouldn't be consistent with what make SPKG does for installable packages: When the system package is in use, make SPKG will nevertheless build the SPKG.

We unfortunately do not have make targets that would mean "make sure that SPKG exists, either as system package or a built package" -- see #31501 for this

comment:17 follow-up: Changed 7 days ago by jhpalmieri

Although it would be consistent with what we do for an installable package when the system package is not in use (or does not exist): if it's already built, make PKG doesn't build it again, I get "Nothing to (re)build / all up-to-date." Should we treat an uninstallable system package like that? Anyway, I agree, that's #31501.

comment:18 in reply to: ↑ 17 Changed 7 days ago by mkoeppe

Replying to jhpalmieri:

Although it would be consistent with what we do for an installable package when the system package is not in use (or does not exist): if it's already built, make PKG doesn't build it again, I get "Nothing to (re)build / all up-to-date." Should we treat an uninstallable system package like that?

No, I think that would be confusing

comment:19 Changed 7 days ago by jhpalmieri

I guess --with-system-perl_cpan_polymake_prereq still makes sense, in particular with "force", but --with-system-perl_cpan_polymake_prereq=no doesn't. Not a big deal.

comment:20 follow-up: Changed 7 days ago by jhpalmieri

I've lost track of the flow in Makefile.in. Why check -x .../spkg-install and not check whether spkg-install.in exists? Are the packages with spkg-install.in managed elsewhere?

comment:21 in reply to: ↑ 20 Changed 7 days ago by mkoeppe

Replying to jhpalmieri:

I've lost track of the flow in Makefile.in. Why check -x .../spkg-install and not check whether spkg-install.in exists? Are the packages with spkg-install.in managed elsewhere?

Script packages don't use ".in" files, see https://doc.sagemath.org/html/en/developer/packaging.html#install-scripts-of-script-packages

comment:22 Changed 7 days ago by mkoeppe

(There's also a ticket to unify this between script and normal packages. #29386)

comment:23 Changed 7 days ago by jhpalmieri

  • Reviewers changed from https://github.com/sagemath/sagetrac-mirror/actions/runs/1249185115 to John Palmieri
  • Status changed from needs_review to positive_review

Thank you for answering all of my questions. Let's merge this.

comment:24 Changed 7 days ago by jhpalmieri

Can we use this treatment in #31529?

comment:25 Changed 7 days ago by mkoeppe

Thank you!

Note: See TracTickets for help on using tickets.