Opened 3 years ago

Closed 11 months ago

#29114 closed enhancement (fixed)

Only ./bootstrap should glob build/pkgs

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.6
Component: build: configure Keywords:
Cc: dimpase, embray, mjo, jhpalmieri Merged in:
Authors: Matthias Koeppe Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: e7b8c3b (Commits, GitHub, GitLab) Commit: e7b8c3b398eba7721df7c79d8d4f77ca454838e8
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by mkoeppe)

Right now both ./bootstrap and ./configure (via m4/sage_spkg_collect.m4) glob build/pkgs.

In this ticket, we move this code into bootstrap, making the generated configure script more static, more straightforward to read and debug, and slightly faster because less work (and file system accesses) is done at the run time of configure.

Before:

$ time ./configure -q --without-system-ntl
real	0m40.860s
user	0m24.260s
sys	0m16.266s

After:

real	0m32.485s
user	0m20.773s
sys	0m10.462s

There are no changes in functionality.

We make a cosmetic fix to the ./configure --help output, fixing https://trac.sagemath.org/ticket/33345#comment:20.

Even after this ticket, package-version.txt and dependencies are processed at configure run time, not at bootstrap time. So simple package upgrades can still be done by developers without having to run ./bootstrap.

Change History (54)

comment:1 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:2 Changed 3 years ago by vbraun

Priority: majorblocker

comment:3 Changed 3 years ago by mkoeppe

Authors: Matthias Koeppe

comment:4 Changed 3 years ago by mkoeppe

Dependencies: #28095

comment:5 Changed 3 years ago by mkoeppe

Branch: u/mkoeppe/no-spkg-globbing-in-configure

comment:6 Changed 3 years ago by mkoeppe

Commit: f7a84e0861cf588f37938630ff4773b2c8b59e0a
Status: newneeds_review

Branch is on top of #28095.


New commits:

cf5607eAdd configure options '--enable-SPKG', --disable-SPKG' for optional/experimental SPKG
e4675d1Actually implement --enable-SPKG and --disable-SPKG
a3e0e47Change type of gmp, mpir to 'standard' (they are mutually exclusive standard packages)
4ed7b05Fix indentation of '--with-mp=' options in configure --help
82ee84dsage_spkg_enable.m4: Popdef what wass pushdef-ed
f7a84e0Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing

comment:7 Changed 3 years ago by git

Commit: f7a84e0861cf588f37938630ff4773b2c8b59e0a355d1476f591e351a387c5acd5ee223b4407c674

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

6ec6413Merge branch 'develop' of git://trac.sagemath.org/sage into t/29114/no-spkg-globbing-in-configure
9295a74sage_spkg_enable.m4: Popdef what wass pushdef-ed
355d147Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing

comment:8 Changed 3 years ago by git

Commit: 355d1476f591e351a387c5acd5ee223b4407c674bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b8

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

bbd6521Makefile [configure]: also depend on build/pkgs/type

comment:9 Changed 3 years ago by dimpase

Could you explain what "glob" means here?

comment:10 Changed 3 years ago by dimpase

This reminds me of #27373 - which becomes obsolete as soon as this ticket and #29113 is done.

comment:11 Changed 3 years ago by embray

It seems this is adding a bunch of previously unnecessary complication only because #29038, which I strongly objected to, was merged. I didn't step in then because I was busy with other things. Can we please roll back #29038 instead? I'm absolute convinced there's no need for it.

comment:12 Changed 3 years ago by embray

Status: needs_reviewneeds_info

comment:13 Changed 3 years ago by embray

See #29118 for an alternative proposal.

comment:14 Changed 3 years ago by dimpase

Status: needs_infoneeds_review

I think we should progress, even if the route is not optimal, and Matthias has a lot of time for working on the build system now.

I hate to see ./configure running many times as I launch make, it really slows working on build system down, and I hope #29113 will happen soon.

comment:15 Changed 3 years ago by embray

I should add, some of the changes in this ticket look reasonable. But it's mixed up with a bunch of other stuff (the way this is built on top of #28095 in particular is confusing). I'd like to start with #29118 and then see some fixes to individual bugs that predate #29038 .

Last edited 3 years ago by embray (previous) (diff)

comment:16 in reply to:  14 Changed 3 years ago by embray

Replying to dimpase:

I think we should progress, even if the route is not optimal, and Matthias has a lot of time for working on the build system now.

Progress on what though? I don't think you can "progress" on unsolid footing. I should note that part of the motivation behind #29038 is to enable the approach in #27824 which I also think is overcomplicated.

I don't think we should be making complicated changes on top of complicated changes to enable even more complicated changes. I hate to sound like Jeroen here because I've been on the other side of disagreements like this with him. But he'd probably be saying the same if he were here and I'd agree this time.

I would like to know what the bigger picture is to all this before making changes that don't seem well-motivated. Don't commit a sunk cost fallacy especially when it's relatively early...

Last edited 3 years ago by embray (previous) (diff)

comment:17 Changed 3 years ago by dimpase

If you want to talk about simplification, why don't we just drop sage -i/-f instead of trying to keep this awful hack - something I proposed a year ago, and Jeroen objected...

comment:18 Changed 3 years ago by mkoeppe

Dependencies: #28095#28095, #29120
Description: modified (diff)

comment:19 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:20 Changed 3 years ago by git

Commit: bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b87214ed7635d37a03827692a2cac666d938265da0

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

1f193bfIssue only a warning, not an error, if there are stray files in build/pkgs
a04733aMerge branch 't/29120/one-line-fix-configure-too-sensitive-to-stray-files-in-build-pkgs' into t/29114/no-spkg-globbing-in-configure
25a55e0Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing
7214ed7Makefile [configure]: also depend on build/pkgs/type

comment:21 Changed 3 years ago by mkoeppe

I've rebased on top of #29120, which has the "hot fix", which may be easier to review and should go into the next beta.

The present ticket, #29114, is not so urgent.

comment:22 Changed 3 years ago by mkoeppe

Priority: blockermajor

comment:23 Changed 3 years ago by mkoeppe

Cc: mjo added

comment:24 Changed 3 years ago by mkoeppe

Status: needs_reviewneeds_work

comment:25 Changed 3 years ago by mjo

Since this list changes maybe once a year, we could just write it into configure.ac and avoid that part of the bootstrap process.

comment:26 Changed 3 years ago by mkoeppe

I think that may be too radical a change (although I am personally in favor of it - agreeing with the autotools policies) - Sage has traditionally used source discovery instead of listing files explicitly. That's why in this ticket I just want to put the source discovery for build/pkgs into 1 script

comment:27 Changed 3 years ago by mkoeppe

Milestone: sage-9.1sage-9.2

comment:28 Changed 2 years ago by mkoeppe

Milestone: sage-9.2sage-9.3

comment:29 Changed 2 years ago by mkoeppe

Milestone: sage-9.3sage-9.4

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

comment:30 Changed 21 months ago by mkoeppe

Milestone: sage-9.4sage-wishlist

comment:31 Changed 12 months ago by git

Commit: 7214ed7635d37a03827692a2cac666d938265da00f59e298cb9901899918d35a5a98132711266285

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

0f59e29Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing

comment:32 Changed 12 months ago by mkoeppe

Milestone: sage-wishlistsage-9.6

comment:33 Changed 12 months ago by mkoeppe

Dependencies: #28095, #29120

comment:34 Changed 12 months ago by mkoeppe

Description: modified (diff)

comment:35 Changed 12 months ago by git

Commit: 0f59e298cb9901899918d35a5a98132711266285c5c6caa717a3c81c75db6de9d7750b10b14170b3

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

a34f10bbootstrap, m4/sage_spkg_collect.m4: Pass SPKG_TYPE to SAGE_SPKG
025eb95bootstrap: Improve the SPKG.rst title parser
5813473bootstrap: First SAGE_SPKG_CONFIGURE, then SAGE_SPKG_FINALIZE
c5c6caam4/sage_spkg_collect.m4 (SAGE_SPKG_COLLECT_INIT): Rename from SAGE_SPKG_INIT

comment:36 Changed 12 months ago by git

Commit: c5c6caa717a3c81c75db6de9d7750b10b14170b354974a3dd43a860710eedeab330aff44309117f1

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

54974a3m4/sage_spkg_collect.m4 (SAGE_SPKG_COLLECT_INIT): Rename from SAGE_SPKG_INIT

comment:37 Changed 12 months ago by git

Commit: 54974a3dd43a860710eedeab330aff44309117f17310bd5d2760ad3774d6c99903ff73498b5c2ca7

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

2eba1f4m4/sage_spkg_collect.m4 (SAGE_SPKG_FINALIZE): Make SPKG_NAME, SPKG_TYPE m4 definitions, not shell variables
1e9c638m4/sage_spkg_collect.m4: Distinguish cases of SPKG_TYPE at macroexpansion time, not runtime
7310bd5bootstrap: First SAGE_SPKG_CONFIGURE, then SAGE_SPKG_FINALIZE (fixup)

comment:38 Changed 12 months ago by git

Commit: 7310bd5d2760ad3774d6c99903ff73498b5c2ca71eceb523dcc007ac78da9d0843000c49f0c14ded

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

1eceb52bootstrap: Determine SPKG_SOURCE here, pass it as a macro argument to SAGE_SPKG_FINALIZE

comment:39 Changed 12 months ago by git

Commit: 1eceb523dcc007ac78da9d0843000c49f0c14ded7115a72c347984e9055d8b58cc6b949b16374629

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

a013698m4/sage_spkg_collect.m4: Distinguish underscore packages at macroexpansion time
8421854m4/sage_spkg_collect.m4: Handle in_sdist at macroexpansion time
7115a72m4/sage_spkg_collect.m4: Suppress some whitespace in the macroexpansion

comment:40 Changed 12 months ago by git

Commit: 7115a72c347984e9055d8b58cc6b949b16374629be824d6ad187241766b14eb182be4f07c46628e2

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

35fa89fbootstrap: Determine SPKG_TREE_VAR here, pass it as a macro argument to SAGE_SPKG_FINALIZE
8070dd9m4/sage_spkg_collect.m4: Make want_spkg a macro definition only
be824d6m4/sage_spkg_collect.m4: Replace read from requirements.txt by use of macro argument

comment:41 Changed 12 months ago by git

Commit: be824d6ad187241766b14eb182be4f07c46628e2e796fcfed31476067722164c6a70871b588232ef

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

e796fcfm4/sage_spkg_enable.m4: Do not AC_REQUIRE([SAGE_SPKG_COLLECT_INIT]) here

comment:42 Changed 12 months ago by mkoeppe

Cc: jhpalmieri added
Description: modified (diff)
Status: needs_workneeds_review

comment:43 Changed 12 months ago by git

Commit: e796fcfed31476067722164c6a70871b588232efa6d70c1ee37f5d199dc03df31cb5d352258c65f9

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

a6d70c1m4/sage_spkg_collect.m4: Make SAGE_NEED_SYSTEM_PACKAGES_VAR a macro

comment:44 Changed 12 months ago by git

Commit: a6d70c1ee37f5d199dc03df31cb5d352258c65f9e7b8c3b398eba7721df7c79d8d4f77ca454838e8

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

7d77884bootstrap: More "installing..."
3b04496bootstrap: Consolidate output redirections
e7b8c3bm4/sage_spkg_collect.m4: Add missing m4_popdef

comment:45 Changed 12 months ago by jhpalmieri

./configure -q is a useful command. I see this output:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named lzma
./configure: line 39392: ,: command not found

The first is probably coming from the line if "$SAGE_BOOTSTRAP_PYTHON" -c "import lzma"; then :, no big deal. The other error is from a comma all by itself on line 39392 (that's with develop; it's line 42361 with this branch). Maybe an extra comma in build/pkgs/qhull/spkg-configure.m4?

comment:46 Changed 12 months ago by mkoeppe

This is unrelated to the ticket and fixed in #33247

comment:47 Changed 12 months ago by mkoeppe

Description: modified (diff)

comment:48 Changed 12 months ago by mkoeppe

Description: modified (diff)

comment:49 Changed 12 months ago by jhpalmieri

In principle this looks good to me, and Sage builds. It would be great if someone more familiar with .m4 code could check the details.

comment:50 Changed 11 months ago by mjo

I looked through it all and don't see anything too scary. My one question is about the grep -v ^= that was added to bootstrap. It looks like that only affects the sagemath_categories, sagemath_objects, and sage_docbuild packages. Why do those have ======... headers in their SPKG.rst to begin with?

comment:51 in reply to:  50 Changed 11 months ago by mkoeppe

Replying to mjo:

It looks like that only affects the sagemath_categories, sagemath_objects, and sage_docbuild packages. Why do those have ======... headers in their SPKG.rst to begin with?

These files double as README.rst for these distributions, so this affects how it displays on PyPI.

comment:52 Changed 11 months ago by mjo

Reviewers: Michael Orlitzky
Status: needs_reviewpositive_review

Ok, I don't believe in parsing SPKG.rst for package information anyway, so I don't mind the hack. (In the futue, I would rather we quit parsing that file than try to make them all nice and consistent.)

Everything else looks reasonable. Nothing broke in my sage build and there aren't any new subtle ./configure errors logged to the console.

John, don't forget to add yourself as a reviewer if you want to.

comment:53 Changed 11 months ago by mkoeppe

Thanks.

comment:54 Changed 11 months ago by vbraun

Branch: u/mkoeppe/no-spkg-globbing-in-configuree7b8c3b398eba7721df7c79d8d4f77ca454838e8
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.