Opened 21 months ago

Last modified 5 months ago

#29114 needs_work enhancement

Only ./bootstrap should glob build/pkgs

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-wishlist
Component: build: configure Keywords:
Cc: dimpase, embray, mjo Merged in:
Authors: Matthias Koeppe Reviewers:
Report Upstream: N/A Work issues:
Branch: u/mkoeppe/no-spkg-globbing-in-configure (Commits, GitHub, GitLab) Commit: 7214ed7635d37a03827692a2cac666d938265da0
Dependencies: #28095, #29120 Stopgaps:

Status badges

Description (last modified by mkoeppe)

Right now both ./bootstrap and ./configure (via m4/sage_spkg_collect.m4) glob 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.

Change History (30)

comment:1 Changed 21 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 21 months ago by vbraun

  • Priority changed from major to blocker

comment:3 Changed 21 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:4 Changed 21 months ago by mkoeppe

  • Dependencies set to #28095

comment:5 Changed 21 months ago by mkoeppe

  • Branch set to u/mkoeppe/no-spkg-globbing-in-configure

comment:6 Changed 21 months ago by mkoeppe

  • Commit set to f7a84e0861cf588f37938630ff4773b2c8b59e0a
  • Status changed from new to needs_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 21 months ago by git

  • Commit changed from f7a84e0861cf588f37938630ff4773b2c8b59e0a to 355d1476f591e351a387c5acd5ee223b4407c674

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 21 months ago by git

  • Commit changed from 355d1476f591e351a387c5acd5ee223b4407c674 to bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b8

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

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

comment:9 Changed 21 months ago by dimpase

Could you explain what "glob" means here?

comment:10 Changed 21 months ago by dimpase

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

comment:11 Changed 21 months 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 21 months ago by embray

  • Status changed from needs_review to needs_info

comment:13 Changed 21 months ago by embray

See #29118 for an alternative proposal.

comment:14 follow-up: Changed 21 months ago by dimpase

  • Status changed from needs_info to 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 21 months 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 21 months ago by embray (previous) (diff)

comment:16 in reply to: ↑ 14 Changed 21 months 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 21 months ago by embray (previous) (diff)

comment:17 Changed 21 months 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 21 months ago by mkoeppe

  • Dependencies changed from #28095 to #28095, #29120
  • Description modified (diff)

comment:19 Changed 21 months ago by mkoeppe

  • Description modified (diff)

comment:20 Changed 21 months ago by git

  • Commit changed from bbd65219cf1eac1f6f88883d3b0d14ff4d9d47b8 to 7214ed7635d37a03827692a2cac666d938265da0

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 21 months 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 21 months ago by mkoeppe

  • Priority changed from blocker to major

comment:23 Changed 19 months ago by mkoeppe

  • Cc mjo added

comment:24 Changed 19 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:25 Changed 19 months 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 19 months 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 19 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:28 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:29 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:30 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-wishlist
Note: See TracTickets for help on using tickets.