Only ./bootstrap should glob build/pkgs #29114

mkoeppe · 2020-01-30T20:33:01Z

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

The text was updated successfully, but these errors were encountered:

mkoeppe · 2020-01-31T00:44:23Z

Author: Matthias Koeppe

mkoeppe · 2020-01-31T01:08:46Z

Dependencies: #28095

mkoeppe · 2020-01-31T01:12:53Z

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

mkoeppe · 2020-01-31T01:13:19Z

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`

mkoeppe · 2020-01-31T01:13:19Z

Commit: f7a84e0

sagetrac-git · 2020-01-31T01:22:36Z

Changed commit from f7a84e0 to 355d147

sagetrac-git · 2020-01-31T01:22:36Z

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`

sagetrac-git · 2020-01-31T02:16:18Z

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

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

sagetrac-git · 2020-01-31T02:16:18Z

Changed commit from 355d147 to bbd6521

dimpase · 2020-01-31T09:33:42Z

comment:9

Could you explain what "glob" means here?

dimpase · 2020-01-31T09:48:46Z

comment:10

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

embray · 2020-01-31T11:24:18Z

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.

embray · 2020-01-31T11:35:54Z

comment:13

See #29118 for an alternative proposal.

dimpase · 2020-01-31T11:39:30Z

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.

embray · 2020-01-31T11:56:03Z

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 .

embray · 2020-01-31T11:59:18Z

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

dimpase · 2020-01-31T12:09:22Z

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

sagetrac-git · 2022-02-17T23:39:51Z

Changed commit from 7310bd5 to 1eceb52

sagetrac-git · 2022-02-18T00:42:13Z

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`

sagetrac-git · 2022-02-18T00:42:13Z

Changed commit from 1eceb52 to 7115a72

sagetrac-git · 2022-02-18T01:22:36Z

Changed commit from 7115a72 to be824d6

sagetrac-git · 2022-02-18T01:22:36Z

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`

sagetrac-git · 2022-02-18T02:04:39Z

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`

sagetrac-git · 2022-02-18T02:04:39Z

Changed commit from be824d6 to e796fcf

sagetrac-git · 2022-02-18T02:57:58Z

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`

sagetrac-git · 2022-02-18T02:57:58Z

Changed commit from e796fcf to a6d70c1

sagetrac-git · 2022-02-18T04:51:48Z

Changed commit from a6d70c1 to e7b8c3b

sagetrac-git · 2022-02-18T04:51:48Z

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`

jhpalmieri · 2022-02-18T06:13:37Z

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?

mkoeppe · 2022-02-18T06:14:57Z

comment:46

This is unrelated to the ticket and fixed in #33247

jhpalmieri · 2022-02-24T23:31:24Z

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.

orlitzky · 2022-02-28T16:12:18Z

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?

mkoeppe · 2022-02-28T18:26:26Z

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.

orlitzky · 2022-02-28T18:51:36Z

Reviewer: Michael Orlitzky

orlitzky · 2022-02-28T18:51:36Z

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.

mkoeppe · 2022-02-28T18:54:09Z

comment:53

Thanks.

vbraun · 2022-03-01T21:31:38Z

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

mkoeppe added this to the sage-9.1 milestone Jan 30, 2020

mkoeppe added c: build: configure labels Jan 30, 2020

This comment has been minimized.

Sign in to view

vbraun added p: blocker / 1 and removed p: major / 3 labels Jan 31, 2020

mkoeppe added the s: needs review label Jan 31, 2020

embray added s: needs info and removed s: needs review labels Jan 31, 2020

dimpase added s: needs review and removed s: needs info labels Jan 31, 2020

This comment has been minimized.

Sign in to view

mkoeppe added s: needs review and removed s: needs work labels Feb 18, 2022

This comment has been minimized.

Sign in to view

orlitzky added s: positive review and removed s: needs review labels Feb 28, 2022

vbraun removed the s: positive review label Mar 1, 2022

vbraun closed this as completed in 2a16069 Mar 1, 2022

embray mentioned this issue Jun 24, 2021

Please roll back #29038 #29118

Closed

Only ./bootstrap should glob build/pkgs #29114

Only ./bootstrap should glob build/pkgs #29114

Comments

mkoeppe commented Jan 30, 2020

This comment has been minimized.

mkoeppe commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

sagetrac-git mannequin commented Jan 31, 2020

sagetrac-git mannequin commented Jan 31, 2020

sagetrac-git mannequin commented Jan 31, 2020

sagetrac-git mannequin commented Jan 31, 2020

dimpase commented Jan 31, 2020

dimpase commented Jan 31, 2020

embray commented Jan 31, 2020

embray commented Jan 31, 2020

dimpase commented Jan 31, 2020

embray commented Jan 31, 2020

embray commented Jan 31, 2020

dimpase commented Jan 31, 2020

This comment has been minimized.

sagetrac-git mannequin commented Feb 17, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

This comment has been minimized.

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

sagetrac-git mannequin commented Feb 18, 2022

jhpalmieri commented Feb 18, 2022

mkoeppe commented Feb 18, 2022

This comment has been minimized.

This comment has been minimized.

jhpalmieri commented Feb 24, 2022

orlitzky commented Feb 28, 2022

mkoeppe commented Feb 28, 2022

orlitzky commented Feb 28, 2022

orlitzky commented Feb 28, 2022

mkoeppe commented Feb 28, 2022

vbraun commented Mar 1, 2022