Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Require ./configure before make #29316

Closed
mkoeppe opened this issue Mar 11, 2020 · 34 comments
Closed

Require ./configure before make #29316

mkoeppe opened this issue Mar 11, 2020 · 34 comments
Labels
c: build p: major / 3 t: enhancement
Milestone
sage-9.2

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Mar 11, 2020

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
@mkoeppe mkoeppe added this to the sage-9.1 milestone Mar 11, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 9, 2020
@jhpalmieri
Copy link
Member

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.

@nbruin
Copy link
Contributor

nbruin commented Apr 18, 2020

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.

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

Branch: u/mkoeppe/require___configure_before_make

@dimpase
Copy link
Member

dimpase commented Apr 19, 2020

Commit: 53361d7

@dimpase
Copy link
Member

dimpase commented Apr 19, 2020

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2020

comment:7

I'd have left SAGE_PORT stuff in place.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

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!

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

comment:9

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2020

comment:10

OK, can we have it in 9.1?

@dimpase dimpase modified the milestones: sage-9.2, sage-9.1 Apr 19, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

comment:12

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2020

comment:13

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

@jhpalmieri
Copy link
Member

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

comment:15

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 19, 2020

Work Issues: rebase on top of #29533

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 19, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2020

Changed commit from 53361d7 to ee97240

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 21, 2020

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 21, 2020

Dependencies: #29533

@jhpalmieri
Copy link
Member

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 23, 2020

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.

@dimpase
Copy link
Member

dimpase commented Apr 23, 2020

Reviewer: Dima Pasechnik, John Palmieri

@dimpase
Copy link
Member

dimpase commented Apr 23, 2020

comment:21

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

@jhpalmieri
Copy link
Member

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.

@jhpalmieri
Copy link
Member

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2020

Changed commit from ee97240 to 5ef6910

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2020

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 14, 2020

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 14, 2020

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 14, 2020

Changed dependencies from #29533 to none

@dimpase
Copy link
Member

dimpase commented Jun 15, 2020

comment:29

lgtm

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 15, 2020

comment:30

Thank you!

@vbraun
Copy link
Member

vbraun commented Jun 22, 2020

Changed branch from u/mkoeppe/require___configure_before_make to 5ef6910

@vbraun vbraun closed this as completed in bcb239a Jun 22, 2020
@mkoeppe mkoeppe mentioned this issue Apr 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: build p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-9.2
Development

No branches or pull requests
5 participants
@vbraun @dimpase @jhpalmieri @nbruin @mkoeppe