sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

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

Closed mkoeppe closed 6 years ago

mkoeppe commented 8 years ago

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

Depends on #24729 Depends on #24703

Component: build: configure

Author: Erik Bray

Branch: 2a99185

Reviewer: Julian Rüth

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

embray commented 6 years ago
comment:41

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 :)

embray commented 6 years ago
comment:42

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 7668fcd to 735fd36

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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
embray commented 6 years ago
comment:44

Rebased to incorporate #24729

embray commented 6 years ago

Dependencies: #24729

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 735fd36 to 1c02a0e

embray commented 6 years ago
comment:46

Rebased again and incorporated #24703.

embray commented 6 years ago

Changed dependencies from #24729 to #24729, #24703

fchapoton commented 6 years ago
comment:47

needs rebase

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 1c02a0e to 6f99aab

embray commented 6 years ago
comment:49

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

saraedum commented 6 years ago
comment:50

Ok. Reviewing this now…

saraedum commented 6 years ago

Changed work issues from minor documentation issues to none

saraedum commented 6 years ago
comment:51

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.

embray commented 6 years ago
comment:52

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 6f99aab to bb9ff43

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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.
embray commented 6 years ago
comment:54

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

embray commented 6 years ago
comment:55

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from bb9ff43 to 2a99185

saraedum commented 6 years ago
comment:58

Ok. Your recent changes look good to me.

vbraun commented 6 years ago

Changed branch from u/embray/build/makefile-in to 2a99185

jhpalmieri commented 6 years ago

Changed commit from 2a99185 to none

jhpalmieri commented 6 years ago
comment:60

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.

embray commented 6 years ago
comment:61

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.

embray commented 6 years ago
comment:62

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

embray commented 6 years ago
comment:63

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.

jdemeyer commented 6 years ago
comment:65

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

jdemeyer commented 6 years ago
comment:66

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

embray commented 6 years ago
comment:67

It's not in the latest beta yet.

embray commented 6 years ago
comment:68

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.

embray commented 6 years ago
comment:69

Followup in #24961.

jhpalmieri commented 6 years ago
comment:70

Thanks for the quick diagnosis, by the way!

embray commented 6 years ago
comment:71

Replying to @jhpalmieri:

Thanks for the quick diagnosis, by the way!

No, thank you for pointing it out.

jdemeyer commented 6 years ago
comment:72

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

jdemeyer commented 6 years ago
comment:73

This might have broken $(SAGERUNTIME): #24995

embray commented 6 years ago
comment:74

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

jdemeyer commented 6 years ago
comment:75

DUMMY_PACKAGES seems broken: #25188

jdemeyer commented 6 years ago
comment:76

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.