Opened 3 years ago

Closed 21 months ago

Last modified 18 months ago

#21524 closed enhancement (fixed)

configure.ac: write build/make/Makefile within an AC_CONFIG_FILE, not during main configure

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.2
Component: build: configure Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 2a99185 (Commits) Commit:
Dependencies: #24729, #24703 Stopgaps:

Description (last modified by jdemeyer)

build/make/Makefile should be written by config.status within an AC_CONFIG_FILE template, not at configure time.

Change History (76)

comment:1 in reply to: ↑ description Changed 3 years ago by jdemeyer

The title "Get rid of file descriptor 7 hack" is different from the description "build/make/Makefile should be written within an AC_CONFIG_COMMANDS procedure". Do you want to fix both things here? Can you please clarify?

And honestly, I think there is nothing wrong with the "file descriptor 7 hack". autoconf itself uses similar hacks internally.

comment:2 Changed 3 years ago by mkoeppe

  • Summary changed from configure.ac: Get rid of file descriptor 7 hack to configure.ac: write build/make/Makefile within an AC_CONFIG_COMMANDS, not during main configure

I've updated the title

comment:3 Changed 3 years ago by mkoeppe

I agree, there's nothing wrong with using a file descriptor to write to a file ;)

comment:4 Changed 3 years ago by mkoeppe

  • Dependencies set to #21479
  • Description modified (diff)

comment:5 Changed 3 years ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 2 years ago by embray

Does it necessarily need to use AC_CONFIG_COMMANDS? Most of the stuff that writes build/make/Makefile could just as easily be moved into its own macro in a separate file in the m4/ directory.

As I see it the main goal here is to simplify the configure.ac by moving more functionality into separate files. But moving the existing code into an m4 macro would take less work, and has the advantage that it would have full access to any other variables set in other parts of the configure script, without having to explicitly pass them on to an external command.

comment:7 Changed 2 years ago by embray

On second thought, I can see why this way would be better, as it's more in spirit with how configure is supposed to work w.r.t. generating files. I'll have to give that some more thought. Maybe there aren't so many variables that need to be passed in.

comment:8 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/build/makefile-in
  • Commit set to db428a360b0b8b0322744186c3a690bb38386574
  • Description modified (diff)
  • Milestone changed from sage-7.4 to sage-8.1
  • Priority changed from minor to major
  • Summary changed from configure.ac: write build/make/Makefile within an AC_CONFIG_COMMANDS, not during main configure to configure.ac: write build/make/Makefile within an AC_CONFIG_FILE, not during main configure

Here's my proposed solution to this (I updated the ticket to mention AC_CONFIG_FILE since that's what this uses).

This adds a build/make/Makefile.in template that is used to generated build/make/Makefile in config.status. The configure script now just collects values for some variables that are inserted into the Makefile template. I tried to keep as much actual Makefile syntax out of the output variables as possible, sticking mostly to just lists of package names that are operated over. Most of the rules for building packages are generated by the Makefile itself in $(eval ...) loops. The end result I think is actually cleaner and easier to understand.

If any of this should be controversial, it may be the use of some more advanced GNU make constructs, the portability of which I'm not sure. This was tested with GNU make 3.81 (about 11 years old), and 4.2.1 (very recent). That said, it's using some relatively advanced GNU make features that may not be available in other makes. I think it's still better, but if need be more of the Makefile generation could be pushed back into configure, but still provided via output variables, instead of writing directly to a file.

Increased the priority since I have other build system work that would likely depend on this, and also because it makes some non-trivial performance improvements, especially on Cygwin.


New commits:

b034b3aFix minor bug in the conway_polynomials dependencies; it should just list the bare package name
f88d3cbUse AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
db428a3Further rewrite of configure.ac to do away with the multiple

comment:9 Changed 2 years ago by embray

  • Status changed from new to needs_review

comment:10 Changed 2 years ago by embray

Weird that the patchbot is failing due to Numpy of all things. I wonder if it has something more to do with the fact that it's building in a temp dir (since this is an "unsafe" ticket) than anything to do with the patch itself, but I'll have to see if I can reproduce...

[numpy-1.12.1] Traceback (most recent call last):
[numpy-1.12.1]   File "<stdin>", line 3, in <module>
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/__init__.py", line 142, in <module>
[numpy-1.12.1]     from . import add_newdocs
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/add_newdocs.py", line 13, in <module>
[numpy-1.12.1]     from numpy.lib import add_newdoc
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/lib/__init__.py", line 18, in <module>
[numpy-1.12.1]     from .polynomial import *
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/lib/polynomial.py", line 20, in <module>
[numpy-1.12.1]     from numpy.linalg import eigvals, lstsq, inv
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/linalg/__init__.py", line 51, in <module>
[numpy-1.12.1]     from .linalg import *
[numpy-1.12.1]   File "/tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/linalg/linalg.py", line 29, in <module>
[numpy-1.12.1]     from numpy.linalg import lapack_lite, _umath_linalg
[numpy-1.12.1] ImportError: /tmp/tmp4YcJD2-sage-git-temp-21524/local/lib/python2.7/site-packages/numpy/linalg/lapack_lite.so: undefined symbol: zungqr_

comment:11 Changed 2 years ago by embray

  • Milestone changed from sage-8.1 to sage-8.2

comment:12 Changed 23 months ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflicts

comment:13 Changed 23 months ago by git

  • Commit changed from db428a360b0b8b0322744186c3a690bb38386574 to 14c25774d47d58979525b77af85df28b94e2223d

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

3f96a65Use AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
14c2577Further rewrite of configure.ac to do away with the multiple

comment:14 Changed 23 months ago by embray

  • Status changed from needs_work to needs_review

comment:15 Changed 23 months ago by git

  • Commit changed from 14c25774d47d58979525b77af85df28b94e2223d to 3ba565f3f2ddf5382a4c7a728b720bc132f6d508

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

3c16cc9Use AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
3ba565fFurther rewrite of configure.ac to do away with the multiple

comment:16 Changed 23 months ago by embray

  • Dependencies #21479 deleted
  • Work issues merge conflicts deleted

comment:17 Changed 23 months ago by jdemeyer

Why? This makes the Makefile much harder to understand for no obvious benefit...

comment:18 Changed 23 months ago by jdemeyer

  • Description modified (diff)

Personally, I find this horrible. I highly value the debuggability of an explicit Makefile.

Of course, that's a very subjective thing so I'm keeping the needs_review status.

comment:19 Changed 23 months ago by embray

I explained all of this in my e-mail...

comment:20 follow-ups: Changed 22 months ago by jdemeyer

Allow me to reboot this discussion in a hopefully constructive way... what would you think of using some high-level programming like Python to write the build/make/Makefile? That would solve the problem that configure.ac is a slow mess. On the other hand, we could write actual Makefile rules instead of using macros.

comment:21 follow-up: Changed 22 months ago by saraedum

  • Reviewers set to Julian Rüth

I think the proposed changes are an improvement. I find the proposed setup easier to read, understand, and modify than the previous echos in configure. It also feels closer to the intended way of using autoconf. I am not sure if the resulting Makefile is harder to read, I don't really see why, in any case, I can see why we would like to get rid of the echo hack which makes it quite annoying to edit configure imho.

Some comments (apart from these minor issues, I would not mind setting this to positive review):

  • I might be totally misunderstanding something but the following comment does not actually apply to the Makefile.in, does it?
    +# This file has been automatically generated by
    +#   /home/embray/src/sagemath/sage/config.status
    +# You should not edit it by hand
    

I think that some general comment about sage-the-distribution would be nice here and some comment on what transforms this file to which other file.

  • I don't really understand the following comment
    +    # If $need_to_install_{PKG_NAME} is set to no, then set inst_<pkgname> to
    +    # some dummy file to skip the installation.
    

So, I guess pkgname and PKG_NAME are the same? But where is inst_<pkgname> being set below? (Only indirectly in Makefile.in, right?)

  • I don't see where you are obscuring anything here. (You copied it over and it should probably go into the .in file.)
    +    # Below we have to obscure A""M_V_at, or autoconf would complain:
    +    # "error: possibly undefined macro: A""M_V_at"
    
  • I would like the configure.ac to contain a # comment at the top which explains how this all works, in particular how this differs from a standard autoconf/automake setup.

comment:22 in reply to: ↑ 20 Changed 22 months ago by saraedum

I think this should be for followup ticket. It's taken about half a year to get from Erik's commits to somebody reviewing this. If we do a complete rewrite of this, I think this is going to take forever. (Btw, you agreed to fix up #20382 11 months ago. It's easy to get distracted and not move at all…. We should not strive for absolute perfection here but go with improvements that introduce good code. And then get better in the next iteration…)

Replying to jdemeyer:

Allow me to reboot this discussion in a hopefully constructive way... what would you think of using some high-level programming like Python to write the build/make/Makefile? That would solve the problem that configure.ac is a slow mess. On the other hand, we could write actual Makefile rules instead of using macros.

comment:23 in reply to: ↑ 20 ; follow-ups: Changed 22 months ago by embray

Replying to jdemeyer:

Allow me to reboot this discussion in a hopefully constructive way... what would you think of using some high-level programming like Python to write the build/make/Makefile? That would solve the problem that configure.ac is a slow mess. On the other hand, we could write actual Makefile rules instead of using macros.

It's funny you suggest that. I've actually been working off-and-on in my spare time on my own Python implementation of make. It has a name but until I feel ready to post the code it's a secret :-) It supports POSIX-compatible Makefiles but it also has its own pure-Python format that lets you write make rules in Python, including all the high-level constructs thereof.

In the meantime I don't really see how that would solve anything though.

comment:24 in reply to: ↑ 23 Changed 22 months ago by jdemeyer

Replying to embray:

In the meantime I don't really see how that would solve anything though.

It would solve exactly the same issues as the ones that you are fixing in your branch, just in a different way.

comment:25 Changed 22 months ago by saraedum

  • Work issues set to minor documentation issues

comment:26 in reply to: ↑ 23 Changed 22 months ago by jdemeyer

Replying to embray:

It's funny you suggest that. I've actually been working off-and-on in my spare time on my own Python implementation of make.

I would be more interested in the converse: turn any Python setup.py into Makefile rules.

comment:27 follow-up: Changed 22 months ago by embray

It would still be much slower just by involving Python. It would still be an (actual!) abuse of autoconf.

comment:28 in reply to: ↑ 21 Changed 22 months ago by embray

Replying to saraedum:

Some comments (apart from these minor issues, I would not mind setting this to positive review):

  • I might be totally misunderstanding something but the following comment does not actually apply to the Makefile.in, does it?
    +# This file has been automatically generated by
    +#   /home/embray/src/sagemath/sage/config.status
    +# You should not edit it by hand
    

Oops, I'm not sure how that got there since I wrote this six months ago :) It might be that I started from editing a generated file exactly as it says not to do. But let me double-check that config.status isn't actually putting that there because that would be a bug, and a bad one at that.

I think that some general comment about sage-the-distribution would be nice here and some comment on what transforms this file to which other file.

That would help, though I'm not sure where to put it. It would be nice to have some documentation about though, as it took me a while to wrap my head around too. It's a bit hard to follow that the top-level Makefile calls build/make/install which calls make again but with build/make/Makefile, which in turn calls sage-spkg to actually build/install packages. I don't have too much of a problem with that aspect of the process but it's not obvious.

  • I don't really understand the following comment
    +    # If $need_to_install_{PKG_NAME} is set to no, then set inst_<pkgname> to
    +    # some dummy file to skip the installation.
    

So, I guess pkgname and PKG_NAME are the same? But where is inst_<pkgname> being set below? (Only indirectly in Makefile.in, right?)

Yes, maybe this could be spelled more consistently.

This definitely deserves better documentation (most of how this works is carried over from the existing Makefile--I really didn't change much...) The actual physical (as in, a real file) targets associate with each package are the "stamp" files installed to $SAGE_LOCAL/var/lib/sage/installed/<pkg_name>-<pkg_version>. Within the Makefile the paths to these files are stored in variables named $(inst_<pkg_name>).

What this is referring to is that for packages which are not actually installed into Sage (e.g. we use the system version), instead of pointing to a real stamp file, the $(inst_<pkg_name>) just points to a dummy file indicating "yes, this dependency is satisfied". This way we don't need to remove that package as a dependency of any other package. It's just trivially satisfied.

Anyways, I'll see if I can make that comment clearer.

  • I don't see where you are obscuring anything here. (You copied it over and it should probably go into the .in file.)
    +    # Below we have to obscure A""M_V_at, or autoconf would complain:
    +    # "error: possibly undefined macro: A""M_V_at"
    

Carried over from the original configure.ac. It's not relevant now anywhere so can just be deleted.

  • I would like the configure.ac to contain a # comment at the top which explains how this all works, in particular how this differs from a standard autoconf/automake setup.

Okay, maybe that's a decent place to put it...

comment:29 Changed 22 months ago by embray

  • Status changed from needs_review to needs_work

comment:30 follow-up: Changed 22 months ago by mjo

While I agree that the templates are a bit incomprehensible, overall I like this change. The separation between configure.ac and Makefile.in makes it more clear what the build is trying to do, with less of a focus on how it's done. I think that in the future, it will be easier to replace chunks of either file, because we won't have to worry about unintended side effects buried in the shell code.

Other ideas:

  1. Our Makefile.in is now GNU-Make-specific. Should we call it GNUMakefile.in to avoid insane errors on systems where "make" isn't GNU?
  2. We should detect SHELL rather than setting it to /bin/bash, because there are systems where it lives in e.g. /usr/bin/bash (https://www.freedesktop.org/wiki/Software/systemd/TheCaseForTheUsrMerge/)
  3. Is Bash needed for the makefile at all? If we replace "source" with ".", does anything else crash with POSIX sh?

comment:31 in reply to: ↑ 27 Changed 22 months ago by jdemeyer

Replying to embray:

It would still be much slower just by involving Python.

Why do you think that? It just needs to read of bunch of files and do some string manipulation. It should be easy.

It would still be an (actual!) abuse of autoconf.

I'm not denying that...

Still, I think it would be fast, work well and produce readable output. So I think that you should at least seriously consider it.

comment:32 follow-up: Changed 22 months ago by embray

Let's give this a try for now (modulo other review comments). I sympathize with your concerns but I strongly suspect that they will prove overstated. If it turns into a concrete problem somehow then I'll reconsider.

comment:33 in reply to: ↑ 30 Changed 22 months ago by embray

Replying to mjo:

While I agree that the templates are a bit incomprehensible, overall I like this change. The separation between configure.ac and Makefile.in makes it more clear what the build is trying to do, with less of a focus on how it's done. I think that in the future, it will be easier to replace chunks of either file, because we won't have to worry about unintended side effects buried in the shell code.

I'm going to try to do a bit of reformatting to make it a little easier to read.

Other ideas:

  1. Our Makefile.in is now GNU-Make-specific. Should we call it GNUMakefile.in to avoid insane errors on systems where "make" isn't GNU?

One thing I'll point out that I haven't explicitly mentioned yet: I asked a few people about this before doing the work (I forget who) but at the time everyone seemed to agree that it would be fine to require GNU make, as there are other packages in Sage that require it, so building the full Sage-the-distribution doesn't work with any other make implementation anyways.

I suppose we could do that though.

  1. We should detect SHELL rather than setting it to /bin/bash, because there are systems where it lives in e.g. /usr/bin/bash (https://www.freedesktop.org/wiki/Software/systemd/TheCaseForTheUsrMerge/)

Right; originally we had SHELL = `command -v bash` here. I'll fix that.

  1. Is Bash needed for the makefile at all? If we replace "source" with ".", does anything else crash with POSIX sh?

I'm not sure it's necessarily strictly needed for the makefile itself, but it in turn calls scripts that require bash, so I don't see any advantage in not just forcing bash in the makefile as well.

comment:34 Changed 22 months ago by embray

One other issue I noticed while working on cleaning this up and adding better debugging tools, is that this doesn't currently handle order-only prerequisites correctly. This was definitely obscured before, but will be easy to fix.

Last edited 22 months ago by embray (previous) (diff)

comment:35 Changed 22 months ago by git

  • Commit changed from 3ba565f3f2ddf5382a4c7a728b720bc132f6d508 to 9ce84c855f234835a6a925052b1f325815aaa142

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

0d12016Remove some bogus comments
9ce84c8Improved (?) documentation of the Makefile.in template.

comment:36 Changed 22 months ago by embray

Last commit made the templates look a bit less noisy. For example, before:

define NORMAL_PACKAGE_templ
$$(INST)/$(1)-$$(vers_$(1)): $$(foreach dep,$$(deps_$(1)),$$(inst_$$(dep)))
	+$(AM_V_at)sage-logger -p '$(SAGE_SPKG) $(1)-$$(vers_$(1))' '$(SAGE_LOGS)/$(1)-$$(vers_$(1)).log'
 
$(1): $$(INST)/$(1)-$$(vers_$(1))
 
$(1)-clean:
	rm -rf $$(INST)/$(1)-$$(vers_$(1))
endef

and after:

define NORMAL_PACKAGE_templ
$$(INST)/$(1)-$(2): $(3)
	+$(AM_V_at)sage-logger -p '$$(SAGE_SPKG) $(1)-$(2)' '$$(SAGE_LOGS)/$(1)-$(2).log'
 
$(1): $$(INST)/$(1)-$(2)
 
$(1)-clean:
	rm -rf $$(INST)/$(1)-$(2)
endef

I also added the DEBUG_RULES flag that I mentioned on sage-devel. If you run the generated makefile in dry-run mode with this flag defined you can see exactly what the templates are outputting:

$ make -f build/make/Makefile -n DEBUG_RULES=1
# Rules for standard packages
$(INST)/4ti2-1.6.7:   $(inst_zlib)  $(inst_mpir)  $(inst_glpk)
        +sage-logger -p '$(SAGE_SPKG) 4ti2-1.6.7' '$(SAGE_LOGS)/4ti2-1.6.7.log'

4ti2: $(INST)/4ti2-1.6.7

4ti2-clean:
        rm -rf $(INST)/4ti2-1.6.7

$(INST)/alabaster-0.7.10:   $(inst_python2)  |  $(inst_pip)
        +sage-logger -p '$(SAGE_SPKG) alabaster-0.7.10' '$(SAGE_LOGS)/alabaster-0.7.10.log'
...
Last edited 22 months ago by embray (previous) (diff)

comment:37 Changed 22 months ago by git

  • Commit changed from 9ce84c855f234835a6a925052b1f325815aaa142 to fc063dcc313ae28b78e4e0030487a2f7a5bbff4e

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

fc063dcGet SHELL from autoconf

comment:38 Changed 22 months ago by git

  • Commit changed from fc063dcc313ae28b78e4e0030487a2f7a5bbff4e to d17e494018a226a4019626d47fc631c9b60fe6ec

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

d796901Delete redundant AC_CONFIG_MACRO_DIR
d17e494Added a little bit more information about the build system and how it works, at a high level

comment:39 Changed 22 months ago by git

  • Commit changed from d17e494018a226a4019626d47fc631c9b60fe6ec to 7668fcd56bbc31f1f57d7f1862d9ed4e5f96dddb

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

7668fcdFix sage -i

comment:40 in reply to: ↑ 32 ; follow-up: Changed 22 months ago by jdemeyer

Replying to embray:

I sympathize with your concerns but I strongly suspect that they will prove overstated.

I really hope that you are right and that this won't turn in a "I told you so" scenario.

I'll stop protesting for now. There is not much to discuss further anyway, we each know the other's opinions.

comment:41 in reply to: ↑ 40 Changed 22 months ago by embray

Replying to jdemeyer:

Replying to embray:

I sympathize with your concerns but I strongly suspect that they will prove overstated.

I really hope that you are right and that this won't turn in a "I told you so" scenario.

If it does, I promise that I'll graciously accept that you were right :)

comment:42 Changed 22 months ago by embray

  • Status changed from needs_work to needs_review

Also, I think that I've addressed most of the other review comments so far....

comment:43 Changed 22 months ago by git

  • Commit changed from 7668fcd56bbc31f1f57d7f1862d9ed4e5f96dddb to 735fd36d2e9db5de174d1166f569370ec6811d2e

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

4b4f40eAdd --with-python=3 configure flag to replace SAGE_PYTHON3=yes
0b36d3eUse AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
e86ea4cFurther rewrite of configure.ac to do away with the multiple
586eb4aRemove some bogus comments
1a04d71Improved (?) documentation of the Makefile.in template.
4dbf15bGet SHELL from autoconf
7cddac9Delete redundant AC_CONFIG_MACRO_DIR
d889070Added a little bit more information about the build system and how it works, at a high level
735fd36Fix sage -i

comment:44 Changed 22 months ago by embray

  • Dependencies set to #24729

Rebased to incorporate #24729

comment:45 Changed 22 months ago by git

  • Commit changed from 735fd36d2e9db5de174d1166f569370ec6811d2e to 1c02a0eb70081bd3b422c1e64beea37cf090fd80

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

057b434Everything should be rebuilt after GCC upgrade
4056a0dMerge branch 'u/jdemeyer/everything_should_be_rebuilt_after_gcc_upgrade' into HEAD
025804cUse AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
83bc9fcFurther rewrite of configure.ac to do away with the multiple
87c7389Remove some bogus comments
283ec7bImproved (?) documentation of the Makefile.in template.
3b512ddGet SHELL from autoconf
7a9fff3Delete redundant AC_CONFIG_MACRO_DIR
869f57aAdded a little bit more information about the build system and how it works, at a high level
1c02a0eFix sage -i

comment:46 Changed 22 months ago by embray

  • Dependencies changed from #24729 to #24729, #24703

Rebased again and incorporated #24703.

comment:47 Changed 21 months ago by chapoton

  • Status changed from needs_review to needs_work

needs rebase

comment:48 Changed 21 months ago by git

  • Commit changed from 1c02a0eb70081bd3b422c1e64beea37cf090fd80 to 6f99aab93d722fbfd3cd82e752b88ec697b77700

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

5b94974Everything should be rebuilt after GCC upgrade
54f3a48Use AC_CONFIG_FILES + AC_OUTPUT to generate build/make/Makefile
bdeca46Further rewrite of configure.ac to do away with the multiple
1150f95Remove some bogus comments
505db02Improved (?) documentation of the Makefile.in template.
c61dba4Get SHELL from autoconf
eefce2cDelete redundant AC_CONFIG_MACRO_DIR
d77e687Added a little bit more information about the build system and how it works, at a high level
6f99aabFix sage -i

comment:49 Changed 21 months ago by embray

  • Status changed from needs_work to needs_review

The longer this stays open the harder it is to keep it maintained because it will conflict with anything that modifies configure.ac.

comment:50 Changed 21 months ago by saraedum

Ok. Reviewing this now…

comment:51 Changed 21 months ago by saraedum

  • Work issues minor documentation issues deleted

Thanks for this very nice summary in the README. If you have checked that this actually works, feel free to set this to positive review.

comment:52 Changed 21 months ago by embray

There's one last change I would like to make, though it shouldn't substantially alter anything. I just want to move all the stuff that collects the packages into its own macro, and move it to a separate file.

I already did this in another branch, but it doesn't make particular sense to leave out of this ticket.

comment:53 Changed 21 months ago by git

  • Commit changed from 6f99aab93d722fbfd3cd82e752b88ec697b77700 to bb9ff43ee1cec2564794acef45d354659314afa7

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

bb9ff43Moved the code that collects up SPKG info into a single SAGE_SPKG_COLLECT macro, along with additional documentation thereof.

comment:54 Changed 21 months ago by embray

  • Status changed from needs_review to positive_review

Will be interesting to see how the buildbots do with this.

comment:55 Changed 21 months ago by embray

  • Status changed from positive_review to needs_work

There appears to be a bug in outputting the package versions during the configure checks. This might have been introduced in my last rebase...

comment:56 Changed 21 months ago by git

  • Commit changed from bb9ff43ee1cec2564794acef45d354659314afa7 to 2a991859c7fdb62357a0136c6c6df8902e677050

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

2a99185This should be quoted to prevent the $1 from being expanded as an argument to the m4 macro

comment:57 Changed 21 months ago by embray

  • Status changed from needs_work to positive_review

comment:58 Changed 21 months ago by saraedum

Ok. Your recent changes look good to me.

comment:59 Changed 21 months ago by vbraun

  • Branch changed from u/embray/build/makefile-in to 2a991859c7fdb62357a0136c6c6df8902e677050
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:60 Changed 21 months ago by jhpalmieri

  • Commit 2a991859c7fdb62357a0136c6c6df8902e677050 deleted

This completely breaks upgrading Sage, it seems to me. On three different OS X machines with 8.2.beta7 installed, upgrading to 8.2.beta8, or in particular just using this branch, results in a bad build/make/Makefile: lines like

deps_4ti2 =  $(SAGE_LOCAL)/bin/gcczlib $(MP_LIBRARY) glpk
deps_alabaster =  $(SAGE_LOCAL)/bin/gcc$(PYTHON) | pip
deps_appnope =  $(SAGE_LOCAL)/bin/gcc$(PYTHON) | pip

Please fix this.

comment:61 Changed 21 months ago by embray

  • Resolution fixed deleted
  • Status changed from closed to new

Indeed, it looks like a bug introduced upon the last rebase. I didn't catch it because it only affects if you are using Sage's gcc spkg.

comment:62 follow-up: Changed 21 months ago by embray

Actually it appears the problem might come from #24703 in the first place, since it seems to misplace the space in the GCC_DEP variable...

comment:63 in reply to: ↑ 62 Changed 21 months ago by embray

Replying to embray:

Actually it appears the problem might come from #24703 in the first place, since it seems to misplace the space in the GCC_DEP variable...

I take that back--the missing space is in my code.

comment:64 Changed 21 months ago by embray

  • Status changed from new to needs_review

comment:65 Changed 21 months ago by jdemeyer

  • Resolution set to fixed
  • Status changed from needs_review to closed

You cannot just re-open a ticket. Only the release manager should do that.

comment:66 Changed 21 months ago by jdemeyer

Especially this ticket must not be re-opened since it has already been released. Just open a new ticket to fix it.

comment:67 Changed 21 months ago by embray

It's not in the latest beta yet.

comment:68 Changed 21 months ago by embray

Oops, apparently it is. If I had thought otherwise I wouldn't have reopened. Strange--I just checked too any I didn't see it.

comment:69 Changed 21 months ago by embray

Followup in #24961.

comment:70 follow-up: Changed 21 months ago by jhpalmieri

Thanks for the quick diagnosis, by the way!

comment:71 in reply to: ↑ 70 Changed 21 months ago by embray

Replying to jhpalmieri:

Thanks for the quick diagnosis, by the way!

No, thank you for pointing it out.

comment:72 follow-up: Changed 21 months ago by jdemeyer

Why is this making changes to build/pkgs/gcc/spkg-install? That might have broken building Sage from scratch if GCC is built.

comment:73 Changed 21 months ago by jdemeyer

This might have broken $(SAGERUNTIME): #24995

comment:74 in reply to: ↑ 72 Changed 21 months ago by embray

Replying to jdemeyer:

Why is this making changes to build/pkgs/gcc/spkg-install? That might have broken building Sage from scratch if GCC is built.

I just noticed this comment. That was not intentional. Probably a mis-resolved conflict (though I don't know why since I never made any edits to that file in the first place).

comment:75 Changed 20 months ago by jdemeyer

DUMMY_PACKAGES seems broken: #25188

comment:76 Changed 18 months ago by jdemeyer

What was the purpose of this change:

@@ -58,11 +58,11 @@ all-sage: \
 # option to make forces all targets to be built unconditionally)
 download-for-sdist: base
        env SAGE_INSTALL_FETCH_ONLY=yes $(MAKE) -B SAGERUNTIME= \
-               $(SDIST_PACKAGES)
+               $(SDIST_PACKAGE_INSTS)
 

It has broken sdists, see #25319.

Note: See TracTickets for help on using tickets.