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: |
Description (last modified by )
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
Description: | modified (diff) |
---|
comment:2 Changed 3 years ago by
Priority: | major → blocker |
---|
comment:3 Changed 3 years ago by
Authors: | → Matthias Koeppe |
---|
comment:4 Changed 3 years ago by
Dependencies: | → #28095 |
---|
comment:5 Changed 3 years ago by
Branch: | → u/mkoeppe/no-spkg-globbing-in-configure |
---|
comment:6 Changed 3 years ago by
Commit: | → f7a84e0861cf588f37938630ff4773b2c8b59e0a |
---|---|
Status: | new → needs_review |
comment:7 Changed 3 years ago by
Commit: | f7a84e0861cf588f37938630ff4773b2c8b59e0a → 355d1476f591e351a387c5acd5ee223b4407c674 |
---|
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
|
comment:8 Changed 3 years ago by
Commit: | 355d1476f591e351a387c5acd5ee223b4407c674 → bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b8 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
bbd6521 | Makefile [configure]: also depend on build/pkgs/type
|
comment:10 Changed 3 years ago by
comment:11 Changed 3 years ago by
comment:12 Changed 3 years ago by
Status: | needs_review → needs_info |
---|
comment:14 follow-up: 16 Changed 3 years ago by
Status: | needs_info → needs_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
comment:16 Changed 3 years ago by
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...
comment:17 Changed 3 years ago by
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
Dependencies: | #28095 → #28095, #29120 |
---|---|
Description: | modified (diff) |
comment:19 Changed 3 years ago by
Description: | modified (diff) |
---|
comment:20 Changed 3 years ago by
Commit: | bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b8 → 7214ed7635d37a03827692a2cac666d938265da0 |
---|
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
|
comment:21 Changed 3 years ago by
comment:22 Changed 3 years ago by
Priority: | blocker → major |
---|
comment:23 Changed 3 years ago by
Cc: | mjo added |
---|
comment:24 Changed 3 years ago by
Status: | needs_review → needs_work |
---|
comment:25 Changed 3 years ago by
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
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
Milestone: | sage-9.1 → sage-9.2 |
---|
comment:28 Changed 2 years ago by
Milestone: | sage-9.2 → sage-9.3 |
---|
comment:29 Changed 2 years ago by
Milestone: | sage-9.3 → sage-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
Milestone: | sage-9.4 → sage-wishlist |
---|
comment:31 Changed 12 months ago by
Commit: | 7214ed7635d37a03827692a2cac666d938265da0 → 0f59e298cb9901899918d35a5a98132711266285 |
---|
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
|
comment:32 Changed 12 months ago by
Milestone: | sage-wishlist → sage-9.6 |
---|
comment:33 Changed 12 months ago by
Dependencies: | #28095, #29120 |
---|
comment:34 Changed 12 months ago by
Description: | modified (diff) |
---|
comment:35 Changed 12 months ago by
Commit: | 0f59e298cb9901899918d35a5a98132711266285 → c5c6caa717a3c81c75db6de9d7750b10b14170b3 |
---|
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
|
comment:36 Changed 12 months ago by
Commit: | c5c6caa717a3c81c75db6de9d7750b10b14170b3 → 54974a3dd43a860710eedeab330aff44309117f1 |
---|
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
|
comment:37 Changed 12 months ago by
Commit: | 54974a3dd43a860710eedeab330aff44309117f1 → 7310bd5d2760ad3774d6c99903ff73498b5c2ca7 |
---|
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)
|
comment:38 Changed 12 months ago by
Commit: | 7310bd5d2760ad3774d6c99903ff73498b5c2ca7 → 1eceb523dcc007ac78da9d0843000c49f0c14ded |
---|
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
|
comment:39 Changed 12 months ago by
Commit: | 1eceb523dcc007ac78da9d0843000c49f0c14ded → 7115a72c347984e9055d8b58cc6b949b16374629 |
---|
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
|
comment:40 Changed 12 months ago by
Commit: | 7115a72c347984e9055d8b58cc6b949b16374629 → be824d6ad187241766b14eb182be4f07c46628e2 |
---|
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
|
comment:41 Changed 12 months ago by
Commit: | be824d6ad187241766b14eb182be4f07c46628e2 → e796fcfed31476067722164c6a70871b588232ef |
---|
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
|
comment:42 Changed 12 months ago by
Cc: | jhpalmieri added |
---|---|
Description: | modified (diff) |
Status: | needs_work → needs_review |
comment:43 Changed 12 months ago by
Commit: | e796fcfed31476067722164c6a70871b588232ef → a6d70c1ee37f5d199dc03df31cb5d352258c65f9 |
---|
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
|
comment:44 Changed 12 months ago by
Commit: | a6d70c1ee37f5d199dc03df31cb5d352258c65f9 → e7b8c3b398eba7721df7c79d8d4f77ca454838e8 |
---|
comment:45 Changed 12 months ago by
./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:47 Changed 12 months ago by
Description: | modified (diff) |
---|
comment:48 Changed 12 months ago by
Description: | modified (diff) |
---|
comment:49 Changed 12 months ago by
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 follow-up: 51 Changed 11 months ago by
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 Changed 11 months ago by
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
Reviewers: | → Michael Orlitzky |
---|---|
Status: | needs_review → positive_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:54 Changed 11 months ago by
Branch: | u/mkoeppe/no-spkg-globbing-in-configure → e7b8c3b398eba7721df7c79d8d4f77ca454838e8 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch is on top of #28095.
New commits:
Add configure options '--enable-SPKG', --disable-SPKG' for optional/experimental SPKG
Actually implement --enable-SPKG and --disable-SPKG
Change type of gmp, mpir to 'standard' (they are mutually exclusive standard packages)
Fix indentation of '--with-mp=' options in configure --help
sage_spkg_enable.m4: Popdef what wass pushdef-ed
Enumerate packages in ./bootstrap; only warn about missing 'type' files; use the generated list in m4/sage_spkg_collect.m4 instead of globbing