sagemath/sage

Require ./configure before make

Closed this issue · 34 comments

With #27351, configure suggests a list of system packages to install.
The printed messages are easy to miss if users rely on make to invoke configure.

In this ticket, we make it an error to invoke make on an un-configured source tree.

sage-devel discussion (April 2020):

CC: @dimpase @vbraun @jhpalmieri

Component: build

Author: Matthias Koeppe

Branch/Commit: 5ef6910

Reviewer: Dima Pasechnik, John Palmieri

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

comment:2

I just started a discussion about this on sage-devel. This ticket could be expanded, or could be part of a family of tickets, which move all calls to ./configure out of Makefile. Then this would solve #29310 and maybe some other tickets with ./configure running too frequently.

comment:3

Ah, ok. Now I see there's actually a rationale for having it an error to run make without configure because you WANT it to be a manual process. It remains that there are plenty of sub-configures run by sage's make anyway.

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@
 
 In this ticket, we make it an error to invoke `make` on an un-`configure`d source tree.
 
+sage-devel discussion (April 2020):
+- https://groups.google.com/d/msg/sage-devel/9gOkmF6rSjY/wEV4WBQABwAJ

Commit: 53361d7

comment:6

Replying to @nbruin:

Ah, ok. Now I see there's actually a rationale for having it an error to run make without configure because you WANT it to be a manual process. It remains that there are plenty of sub-configures run by sage's make anyway.

sub-configures prepare other makefiles, not the main makefile, so this is not recursive, as opposed to what we have at the top level now, where we basically run make && ./configure in a loop until we reach a stable point, from which we can make.


New commits:

a439974Makefile: Exit with error if not configured
53361d7src/doc/en/installation/source.rst: Get rid of SAGE_PORT
comment:7

I'd have left SAGE_PORT stuff in place.

comment:8

SAGE_PORT does not work at all anyway because if configure exits with an error, all the required config files are not written!

comment:9

If you prefer, we can push the SAGE_PORT removal to another ticket and merge in 9.1 already

comment:10

OK, can we have it in 9.1?

Author: Matthias Koeppe

comment:12

I actually meant to merge the SAGE_PORT removal in 9.1
and "require ./configure before make" in 9.2.

comment:13

any reason to delay "./configure before make" ?

comment:14

It's a change in how developers work, and I think it would be better to have a full beta cycle to test it and get people used to it.

comment:15

I agree with John. I've created #29533 for SAGE_PORT.

Work Issues: rebase on top of #29533

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

c9758e9Makefile, src/doc/en/installation/source.rst: Remove defunct SAGE_PORT mechanism
ee97240Makefile: Exit with error if not configured

Changed commit from 53361d7 to ee97240

Changed work issues from rebase on top of #29533 to none

Dependencies: #29533

comment:19

It works with a fresh tarball, but I'm a little surprised that after make distclean, running make does not produce the error message. I guess make distclean doesn't delete config.status. Should it?

comment:20

Replying to @jhpalmieri:

I guess make distclean doesn't delete config.status.

Yes, our distclean is non-standard in this regard. There is a target bdist-clean that deletes that extra file.

Should it?

Hard to tell. I am not sure about the intended meaning of misc-clean, bdist-clean, distclean, bootstrap-clean, maintainer-clean, fast-rebuild-clean, as well as micro_release. Perhaps @vbraun could weigh in here.

Reviewer: Dima Pasechnik, John Palmieri

comment:21

It's a step in the right direction. Adjusting make targets is another story.

comment:22

I basically agree, but the last line in this passage from the top-level README.md must be changed:

The `configure` script itself, if it is not already built, can be generated by
running the `bootstrap` script (the latter requires _GNU autotools_ being installed).
The top-level `Makefile` also takes care of this automatically.

Maybe this whole section of README.md should be changed? I don't know if there are other parts of the documentation we also need to change.

I can certainly delete the line, but someone who knows autotools, configure files, etc., should really read this section and make any necessary changes.

comment:23

Another spot: in the installation guide it says

#. Optional:  Run the configure script to set some options that
   influence the build process.

(Item 6 at http://doc.sagemath.org/html/en/installation/source.html#step-by-step-installation-procedure.)

We can probably just delete the word "Optional" here.

Changed commit from ee97240 to 5ef6910

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

9d9fb2dMakefile: Exit with error if not configured
5ef6910Update README.md
comment:25

Replying to @jhpalmieri:

the last line in this passage from the top-level README.md must be changed:

The `configure` script itself, if it is not already built, can be generated by
running the `bootstrap` script (the latter requires _GNU autotools_ being installed).
The top-level `Makefile` also takes care of this automatically.

Maybe this whole section of README.md should be changed?

Done.

comment:26

Replying to @jhpalmieri:

Another spot: in the installation guide it says

#. Optional:  Run the configure script to set some options that
   influence the build process.

(Item 6 at http://doc.sagemath.org/html/en/installation/source.html#step-by-step-installation-procedure.)

We can probably just delete the word "Optional" here.

Apparently this was already done by another ticket.

Changed dependencies from #29533 to none

comment:29

lgtm

comment:30

Thank you!