Only ./bootstrap should glob build/pkgs

Question

Only ./bootstrap should glob build/pkgs

mkoeppe opened this issue 6 years ago · 61 comments

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 #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.

CC: @dimpase @embray @orlitzky @jhpalmieri

Component: build: configure

Author: Matthias Koeppe

Branch/Commit: e7b8c3b

Reviewer: Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/29114

Answer 1 · 2020-01-30T23:36:47.000Z

Description changed:

--- 
+++ 
@@ -1,3 +1,6 @@
 Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) glob.
 This should be streamlined. 
 
+Currently `./configure` is too sensitive to stray files/subdirectories in `build/pkgs` (such as those that are `.gitignore`'d in another branch), as noted in https://groups.google.com/d/msg/sage-release/eVvjvJ6Iii8/W51ouUKpDwAJ (thread).
+
+

Answer 2 · 2020-01-31T00:44:23.000Z

Author: Matthias Koeppe

Answer 3 · 2020-01-31T01:08:46.000Z

Dependencies: #28095

Answer 4 · 2020-01-31T01:12:53.000Z

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

Answer 5 · 2020-01-31T01:13:19.000Z

comment:6

Branch is on top of #28095.

New commits:

`cf5607e`	`Add configure options '--enable-SPKG', --disable-SPKG' for optional/experimental SPKG`
`e4675d1`	`Actually implement --enable-SPKG and --disable-SPKG`
`a3e0e47`	`Change type of gmp, mpir to 'standard' (they are mutually exclusive standard packages)`
`4ed7b05`	`Fix indentation of '--with-mp=' options in configure --help`
`82ee84d`	`sage_spkg_enable.m4: Popdef what wass pushdef-ed`
`f7a84e0`	`Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing`

Answer 6 · 2020-01-31T01:13:19.000Z

Commit: f7a84e0

Answer 7 · 2020-01-31T01:22:36.000Z

Changed commit from f7a84e0 to 355d147

Answer 8 · 2020-01-31T01:22:36.000Z

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

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

Answer 9 · 2020-01-31T02:16:18.000Z

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

`bbd6521`	`Makefile [configure]: also depend on build/pkgs/type`

Answer 10 · 2020-01-31T02:16:18.000Z

Changed commit from 355d147 to bbd6521

Answer 11 · 2020-01-31T09:33:42.000Z

comment:9

Could you explain what "glob" means here?

Answer 12 · 2020-01-31T09:48:46.000Z

comment:10

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

Answer 13 · 2020-01-31T11:24:18.000Z

comment:11

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.

Answer 14 · 2020-01-31T11:35:54.000Z

comment:13

See #29118 for an alternative proposal.

Answer 15 · 2020-01-31T11:39:30.000Z

comment:14

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.

Answer 16 · 2020-01-31T11:56:03.000Z

comment:15

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 .

Answer 17 · 2020-01-31T11:59:18.000Z

comment:16

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...

Answer 18 · 2020-01-31T12:09:22.000Z

comment:17

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...

Answer 19 · 2020-01-31T14:37:21.000Z

Description changed:

--- 
+++ 
@@ -1,6 +1,8 @@
 Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) glob.
 This should be streamlined. 
 
-Currently `./configure` is too sensitive to stray files/subdirectories in `build/pkgs` (such as those that are `.gitignore`'d in another branch), as noted in https://groups.google.com/d/msg/sage-release/eVvjvJ6Iii8/W51ouUKpDwAJ (thread).
+Currently `./configure` is too sensitive to stray files/subdirectories in `build/pkgs` (such as those that are `.gitignore`'d in another branch), as noted in https://groups.google.com/d/msg/sage-release/eVvjvJ6Iii8/W51ouUKpDwAJ (thread). 
+While #29120 provides a quick workaround, the present ticket has the proper fix.
 
 
+

Answer 20 · 2020-01-31T14:37:21.000Z

Changed dependencies from #28095 to #28095, #29120

Answer 21 · 2020-01-31T14:39:19.000Z

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) glob.
+Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) [glob](https://en.wikipedia.org/wiki/Glob_(programming)) `build/pkgs`.
 This should be streamlined. 
 
 Currently `./configure` is too sensitive to stray files/subdirectories in `build/pkgs` (such as those that are `.gitignore`'d in another branch), as noted in https://groups.google.com/d/msg/sage-release/eVvjvJ6Iii8/W51ouUKpDwAJ (thread).

Answer 22 · 2020-01-31T14:43:26.000Z

Changed commit from bbd6521 to 7214ed7

Answer 23 · 2020-01-31T14:43:26.000Z

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

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

Answer 24 · 2020-01-31T14:45:38.000Z

comment:21

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.

Answer 25 · 2020-03-16T15:00:11.000Z

comment:25

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

Answer 26 · 2020-03-16T15:43:55.000Z

comment:26

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

Answer 27 · 2021-02-13T20:51:01.000Z

comment:29

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

Answer 28 · 2022-02-17T21:39:59.000Z

Changed commit from 7214ed7 to 0f59e29

Answer 29 · 2022-02-17T21:39:59.000Z

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

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

Answer 30 · 2022-02-17T21:41:55.000Z

Changed dependencies from #28095, #29120 to none

Answer 31 · 2022-02-17T21:42:16.000Z

Description changed:

--- 
+++ 
@@ -1,8 +1,3 @@
 Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) [glob](https://en.wikipedia.org/wiki/Glob_(programming)) `build/pkgs`.
 This should be streamlined. 
 
-Currently `./configure` is too sensitive to stray files/subdirectories in `build/pkgs` (such as those that are `.gitignore`'d in another branch), as noted in https://groups.google.com/d/msg/sage-release/eVvjvJ6Iii8/W51ouUKpDwAJ (thread). 
-While #29120 provides a quick workaround, the present ticket has the proper fix.
-
-
-

Answer 32 · 2022-02-17T22:29:13.000Z

Changed commit from 0f59e29 to c5c6caa

Answer 33 · 2022-02-17T22:29:13.000Z

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

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

Answer 34 · 2022-02-17T22:35:41.000Z

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

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

Answer 35 · 2022-02-17T22:35:41.000Z

Changed commit from c5c6caa to 54974a3

Answer 36 · 2022-02-17T23:23:46.000Z

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

`2eba1f4`	`m4/sage_spkg_collect.m4 (SAGE_SPKG_FINALIZE): Make SPKG_NAME, SPKG_TYPE m4 definitions, not shell variables`
`1e9c638`	`m4/sage_spkg_collect.m4: Distinguish cases of SPKG_TYPE at macroexpansion time, not runtime`
`7310bd5`	`bootstrap: First SAGE_SPKG_CONFIGURE, then SAGE_SPKG_FINALIZE (fixup)`

Answer 37 · 2022-02-17T23:23:46.000Z

Changed commit from 54974a3 to 7310bd5

Answer 38 · 2022-02-17T23:39:51.000Z

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

`1eceb52`	`bootstrap: Determine SPKG_SOURCE here, pass it as a macro argument to SAGE_SPKG_FINALIZE`

Answer 39 · 2022-02-17T23:39:51.000Z

Changed commit from 7310bd5 to 1eceb52

Answer 40 · 2022-02-18T00:42:13.000Z

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

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

Answer 41 · 2022-02-18T00:42:13.000Z

Changed commit from 1eceb52 to 7115a72

Answer 42 · 2022-02-18T01:22:36.000Z

Changed commit from 7115a72 to be824d6

Answer 43 · 2022-02-18T01:22:36.000Z

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

`35fa89f`	`bootstrap: Determine SPKG_TREE_VAR here, pass it as a macro argument to SAGE_SPKG_FINALIZE`
`8070dd9`	`m4/sage_spkg_collect.m4: Make want_spkg a macro definition only`
`be824d6`	`m4/sage_spkg_collect.m4: Replace read from requirements.txt by use of macro argument`

Answer 44 · 2022-02-18T02:04:39.000Z

Changed commit from be824d6 to e796fcf

Answer 45 · 2022-02-18T02:04:39.000Z

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

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

Answer 46 · 2022-02-18T02:19:12.000Z

Description changed:

--- 
+++ 
@@ -1,3 +1,23 @@
 Right now both `./bootstrap` and `./configure` (via `m4/sage_spkg_collect.m4`) [glob](https://en.wikipedia.org/wiki/Glob_(programming)) `build/pkgs`.
-This should be streamlined. 
 
+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.
+

Answer 47 · 2022-02-18T02:57:58.000Z

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

`a6d70c1`	`m4/sage_spkg_collect.m4: Make SAGE_NEED_SYSTEM_PACKAGES_VAR a macro`

Answer 48 · 2022-02-18T02:57:58.000Z

Changed commit from e796fcf to a6d70c1

Answer 49 · 2022-02-18T04:51:48.000Z

Changed commit from a6d70c1 to e7b8c3b

Answer 50 · 2022-02-18T04:51:48.000Z

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

`7d77884`	`bootstrap: More "installing..."`
`3b04496`	`bootstrap: Consolidate output redirections`
`e7b8c3b`	`m4/sage_spkg_collect.m4: Add missing m4_popdef`

Answer 51 · 2022-02-18T06:13:37.000Z

comment:45

./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?

Answer 52 · 2022-02-18T06:14:57.000Z

comment:46

This is unrelated to the ticket and fixed in #33247

Answer 53 · 2022-02-20T02:26:09.000Z

Description changed:

--- 
+++ 
@@ -21,3 +21,5 @@
 
 There are no changes in functionality.
 
+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`.
+

Answer 54 · 2022-02-20T02:27:49.000Z

Description changed:

--- 
+++ 
@@ -21,5 +21,7 @@
 
 There are no changes in functionality.
 
+We make a cosmetic fix to the `./configure --help` output, fixing [#33345 comment:20](https://github.com/sagemath/sage/issues/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`.

Answer 55 · 2022-02-24T23:31:24.000Z

comment:49

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.

Answer 56 · 2022-02-28T16:12:18.000Z

comment:50

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?

Answer 57 · 2022-02-28T18:26:26.000Z

comment:51

Replying to @orlitzky:

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.

Answer 58 · 2022-02-28T18:51:36.000Z

Reviewer: Michael Orlitzky

Answer 59 · 2022-02-28T18:51:36.000Z

comment:52

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.

Answer 60 · 2022-02-28T18:54:09.000Z

comment:53

Thanks.

Answer 61 · 2022-03-01T21:31:38.000Z

Changed branch from u/mkoeppe/no-spkg-globbing-in-configure to e7b8c3b