Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18710 closed enhancement (fixed)

Move some make targets to build/Makefile

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: ncohen Merged in:
Authors: Jeroen Demeyer Reviewers: Nathann Cohen, Volker Braun
Report Upstream: N/A Work issues:
Branch: 6db418e (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Move all make targets involving building parts of Sage to build/Makefile.

One advantage is that we can build the documentation in parallel with packages: not every package needs to be installed to build the documentation; in particular R (which takes a while to build) can be built in parallel with the doc.

We also add targets for every package, such that make mypkg would install mypkg with dependency checking (unlike ./sage -i mypkg which does not have dependency checking).

Change History (57)

comment:1 Changed 4 years ago by jdemeyer

  • Dependencies set to #18533

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/move_some_make_targets_to_build_makefile

comment:5 Changed 4 years ago by jdemeyer

  • Commit set to 8c34e8bac50d84c9b3c60a3eb4ef110df5acfb97
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

d2d009fAdd ccache package
5a67cb8Merge commit 'd2d009fbad12994f0f95a93a915b1d2481f688be' into t/18710/move_some_make_targets_to_build_makefile
8c34e8bMove various targets to build/Makefile

comment:6 follow-up: Changed 4 years ago by ncohen

You make many unrelated changes in the same diff and it's rather hard to figure out. Could you split the commits? Having one commit per block of rules that you move from one place to another would help a lot.

Nathann

comment:7 in reply to: ↑ 6 Changed 4 years ago by jdemeyer

  • Dependencies changed from #18533 to #18533, #18715
  • Status changed from needs_review to needs_work

comment:8 Changed 4 years ago by git

  • Commit changed from 8c34e8bac50d84c9b3c60a3eb4ef110df5acfb97 to 4ad1c78a48e3a02def1088c88e62f8f71598b6fb

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

fa53538Move scripts and extcode targets to build/deps
912c50cMerge tag '6.8.beta4' into ticket/18710
4ad1c78Move various targets to build/Makefile

comment:9 Changed 4 years ago by git

  • Commit changed from 4ad1c78a48e3a02def1088c88e62f8f71598b6fb to 9e66cefcbac69534bb25ee04f8de32d2e4f5cafe

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

ac2d008Small clean-up in top-level build system
451dfb1Merge commit 'fa53538bff4dd81efd8635ebafae9b95146ac5e5' into ticket/18716
9e66cefMove various targets to build/Makefile

comment:10 Changed 4 years ago by jdemeyer

I split off 2 tickets: #18715 and #18716 and kept only the core of this ticket here, I hope it's more clear now.

comment:11 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:12 Changed 4 years ago by jdemeyer

  • Dependencies changed from #18533, #18715 to #18533, #18715, #18716

comment:13 Changed 4 years ago by git

  • Commit changed from 9e66cefcbac69534bb25ee04f8de32d2e4f5cafe to 755021600db6bb06415735cf10043b4351a2a86f

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

7550216Build doc first

comment:14 follow-up: Changed 4 years ago by ncohen

Yes yes it is clearer. Though you didn't have to go through the trouble of creating many tickets, splitting the commits would have been enough. I'll review this soon.

Last edited 4 years ago by ncohen (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

Replying to ncohen:

Though you shouldn't have to go through the trouble of creating many tickets

Well, creating a ticket takes essentially no time (certainly much less than splitting up commits).

comment:16 follow-up: Changed 4 years ago by ncohen

What about the packages that install libraries that Sage needs to compile cython modules? They need to be installed if you want their doc to appear.

And unfortunately, I am also talking of optional ones :-/

comment:17 follow-up: Changed 4 years ago by ncohen

+# Defer unknown targets to build/Makefile
+%::
+	$(MAKE) configure logs
+	cd build && ./pipestatus \
+		"./install '$@' 2>&1" \
+		"tee -a ../logs/install.log"

You can't log everything to .install.log regarless of the command that was run.

comment:18 in reply to: ↑ 16 Changed 4 years ago by jdemeyer

Replying to ncohen:

What about the packages that install libraries that Sage needs to compile cython modules? They need to be installed if you want their doc to appear.

Yes, they are handled by the $(SAGERUNTIME) dependency.

And unfortunately, I am also talking of optional ones :-/

Well, we are currently not building documentation for optional extensions anyway, so that doesn't matter.

comment:19 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

You can't log everything to .install.log regarless of the command that was run.

Actually, why not? I don't see a problem with having one "master" log file logging everything. And it can help debugging race conditions if some command fails depending on when it was run.

The individual log files are still generated, so I don't see this as a regression.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 4 years ago by ncohen

The individual log files are still generated, so I don't see this as a regression.

You concatenate in the same file the results of several commands, don't you? How are we meant to know what is the result of what? Wouldn't it be better to have a logfile whose name includes the date or something?

Also, 'install' is ill-chosen. This filename is used whenever the rule that is being used does not exist in the main makefile, and it can be anything (install/docbuild/tests). It also seems that calls to 'make' with a wrong rule will produce output too.

Nathann

comment:21 in reply to: ↑ 20 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

You concatenate in the same file the results of several commands, don't you?

Yes indeed.

How are we meant to know what is the result of what?

You cannot really know that. But if you need to know, just look at the individual log files, which still exist.

Wouldn't it be better to have a logfile whose name includes the date or something? Also, 'install' is ill-chosen.

Well, I think that changing the name of that file is outside the scope of this ticket. That should be done with caution, as this might have consequences for the patchbot/buildbot for example.

This filename is used whenever the rule that is being used does not exist in the main makefile, and it can be anything (install/docbuild/tests).

Well, this ticket is about integrating the build of the documentation into the rest of the build system. So having one log file is consistent with that.

It also seems that calls to 'make' with a wrong rule will produce output too.

True, but that's not really a major problem, is it?

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by ncohen

If having one makefile call the other forces you to accept so many issues with the logfiles, then this temporary design maybe isn't a good idea.

comment:23 in reply to: ↑ 22 Changed 4 years ago by jdemeyer

Replying to ncohen:

If having one makefile call the other forces you to accept so many issues with the logfiles, then this temporary design maybe isn't a good idea.

The only new issue is that the output of make this_target_does_not_exist gets logged in install.log. I do not believe that this is sufficient reason to reject this patch.

All the other issues that you mentioned in 20 are already present in Sage, independent of this ticket.

comment:24 follow-up: Changed 4 years ago by ncohen

Why do you insist on logging the output of a 'forwarded' rule call? Why wouldn't you just let the actual rule hande the logging?

P.S.: How can you even be sure that the final logging rule does not simultaneously output to install.log?

comment:25 in reply to: ↑ 24 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

Why do you insist on logging the output of a 'forwarded' rule call?

I am just keeping this behaviour which already exists in Sage. Changing this is not in the scope of this ticket.

P.S.: How can you even be sure that the final logging rule does not simultaneously output to install.log?

Even if that happens, that's not really a problem (it's perfectly legal to have more than 1 process append to the same file, then output will appear in "random" order, nothing will be lost).

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:26 in reply to: ↑ 25 ; follow-up: Changed 4 years ago by ncohen

I am just keeping this behaviour which already exists in Sage.

Explain what you mean: this logging is done in a rule that you create in this branch.

Last edited 4 years ago by ncohen (previous) (diff)

comment:27 in reply to: ↑ 26 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

this logging is done in a rule created by this branch.

Currently in Sage:

build: logs configure
    +cd build && \
    "../$(PIPE)" \
        "./install all 2>&1" \
        "tee -a ../logs/install.log"
    +./sage -b

With this ticket:

%::
    $(MAKE) configure logs
    +cd build && ./pipestatus \
        "./install '$@' 2>&1" \
        "tee -a ../logs/install.log"

There is no difference at all in how install.log is treated.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:28 Changed 4 years ago by git

  • Commit changed from 755021600db6bb06415735cf10043b4351a2a86f to 5b6ec50788f2cd19f575adf5f2725d77c4ba6cc3

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

5b6ec50Recursive rule should be marked as recursive

comment:29 in reply to: ↑ 27 ; follow-up: Changed 4 years ago by ncohen

There is no difference at all in how install.log is treated.

Be honest for a second Jeroen. There is no difference at all *when make build* is called. For all other calls, there is a difference. In these situations, it would make more sense to not double-log everything. Why don't you move the logging to install.log to the build: rule in the other makefile?

And then, you can remove the logging from this very very generic rule.

Nathann

comment:30 in reply to: ↑ 29 ; follow-up: Changed 4 years ago by jdemeyer

Can you please try to understand the following sentence?

We are already double-logging the whole build process and my ticket does not change that.

There are a few small differences, sure: with #18710, also the docbuild is double-logged. But that's not a fundamental change.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by ncohen

Can you please try to understand the following sentence?

We are already double-logging the whole build process and my ticket does not change that.

Jeroen, I am here to review your ticket. If all you plan on doing is scream whenever your reviewer disagrees with you, your ticket is blocked. Agreed ?

So let's do something constructive: can you tell me why you insist on double-logging in this new rule that you add? Can't you just move this logging to the other makefile?

comment:32 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

Can you please try to understand the following sentence?

We are already double-logging the whole build process and my ticket does not change that.

Jeroen, I am here to review your ticket. If all you plan on doing is scream whenever your reviewer disagrees with you, your ticket is blocked. Agreed ?

It just feels to me like you are complaining without reading my comments. I hope that the bold sentence above will not escape your attention. Anyway, I need to cool down for a while...

comment:33 in reply to: ↑ 32 Changed 4 years ago by ncohen

It just feels to me like you are complaining without reading my comments. I hope that the bold sentence above will not escape your attention.

To me it is an assertion without proof. You tell me that everything is already double-logged, while I have my eyes on a generic rule that you add that double-logs everything automatically. Is it now triple-logged?

I also asked you in my last comment why you wouldn't move this logging to the other file, so that it only applies to the 'build' rule, but you did not answer that.

Anyway, I need to cool down for a while...

Yep.

Nathann

comment:34 follow-up: Changed 4 years ago by jdemeyer

OK, let me try again:

Can't you just move this logging to the other makefile?

There is already logging in the individual rules in build/Makefile. This is logging number 1.

The logging to install.log from Makefile is logging number 2.

I don't see where a "triple logging" could come from.

Note that install.log contains information which is not available from the individual log files:

  1. a listing of environment variables at the start of the build, and other output from build/install.
  2. timing information: it shows all commands in parallel as they are executed. This is actually a feature, since it allows debugging race conditions. With just the individual files, that information is lost.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 4 years ago by ncohen

Yooooooo !

There is already logging in the individual rules in build/Makefile. This is logging number 1.

The logging to install.log from Makefile is logging number 2.

Right. Do we agree that before your branch is applied, this "number 2 logging" in install.log is only performed for 'make build'?

(assuming that we agree on that) With the new fallback %:: rule that you added, all calls to 'make X' that are not defined in SAGE_ROOT/Makefile are forwarded to build/Makefile. Thus, after your branch is applied, the output of make doc-html is logged in install.log.

(assuming that we agree on that) Before your branch is applied, the output of make doc-html was not logged i install.log.

(assuming that we agree on that) Why wouldn't we remove the logging from the %:: rule, and only add it in the make build rule (which is now the make all-build rule from build/Makefile)?

Note that install.log contains information which is not available from the individual log files:

  1. a listing of environment variables at the start of the build, and other output from build/install.
  2. timing information: it shows all commands in parallel as they are executed. This is actually a feature, since it allows debugging race conditions. With just the individual files, that information is lost.

For 'make build' it I agree that this information can make sense.

Nathann

comment:36 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

Right. Do we agree that before your branch is applied, this "number 2 logging" in install.log is only performed for 'make build'?

Yes. However, let me remark that most top-level targets depend on build, so the "build" part of those targets is also logged in install.log.

With the new fallback %:: rule that you added, all calls to 'make X' that are not defined in SAGE_ROOT/Makefile are forwarded to build/Makefile. Thus, after your branch is applied, the output of make doc-html is logged in install.log.

True. But this is a feature, not a bug.

(assuming that we agree on that) Before your branch is applied, the output of make doc-html was not logged in install.log.

Obviously true.

(assuming that we agree on that) Why wouldn't we remove the logging from the %:: rule, and only add it in the make build rule (which is now the make all-build rule from build/Makefile)?

See the last paragraph of 34. Those reasons make sense for make doc as much for make build.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:37 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I'll try to get rid of the explicit running of ./sage -b (which shouldn't be needed).

comment:38 in reply to: ↑ 36 ; follow-ups: Changed 4 years ago by ncohen

Okay. From what you say, it appears to me that:

1) After your branch is applied, "make build" is logged exactly as previously in install.log 2) After your branch is applied, the output of make doc-html is logged in install.log. As it triggers a make build, then the output of make build is logged twice in install.log (once in make doc-html, once in the triggered make build) 3) My problem with your %:: is that is that it covers *any* kind of rule, even those that we will add in the future. Promising right now that any rules that is aliased from Makefile to build/Makefile will be logged in install.log looks too wide for me.

Thus, could you remove this logging from the %:: rule? You could add the same logging in the make all-sage rule. It would also let you add a more specific logging of make doc that would not cover the subsequent call to make build, thus we would not double-log that.

See the last paragraph of 34. Those reasons make sense for make doc as much for make build.

I would have less problem if you were only adding those rules in make doc and make build. Right now you are doing this in a wider scope: you apply then to all rules that are aliased, even those that will be added later.

Nathann

comment:39 Changed 4 years ago by git

  • Commit changed from 5b6ec50788f2cd19f575adf5f2725d77c4ba6cc3 to 4f982a41cbfddf5acd4283a4c9b9473ebcd587d1

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

7ecf1eaUse TABs for indenting in makefiles
7ca1bceMake the Sage library a phony target "sagelib"
4f982a4Only run sage-starts after all-sage finished building

comment:40 in reply to: ↑ 38 Changed 4 years ago by jdemeyer

  • Commit changed from 4f982a41cbfddf5acd4283a4c9b9473ebcd587d1 to a73b6ea46cb620432de86068d1ca58087702f69d

Replying to ncohen:

After your branch is applied, the output of make doc-html is logged in install.log. As it triggers a make build, then the output of make build is logged twice in install.log (once in make doc-html, once in the triggered make build)

I think you are misunderstanding something, because this is not true: make doc-html does not trigger make build.

The documentation is now built similarly to spkgs. The dependencies for the documentation are declared just like the dependencies for packages, see this part of build/Makefile:

# Building the documentation has many dependencies, because all
# documented modules are imported and because we use matplotlib to
# produce plots.
DOC_DEPENDENCIES = sagelib $(INST)/$(SPHINX) $(INST)/$(SAGENB) \
    | $(SAGERUNTIME) $(INST)/$(MAXIMA) $(INST)/$(NETWORKX) \
    $(INST)/$(SCIPY) $(INST)/$(MATPLOTLIB) $(INST)/$(PILLOW)

doc: doc-html

doc-html: $(DOC_DEPENDENCIES)
    cd .. && $(PIPE) "./sage --docbuild --no-pdf-links all html $(SAGE_DOCBUILD_OPTS) 2>&1" "tee -a logs/dochtml.log"

New commits:

a73b6eaTypo

comment:41 in reply to: ↑ 38 Changed 4 years ago by jdemeyer

Replying to ncohen:

3) My problem with your %:: is that is that it covers *any* kind of rule, even those that we will add in the future. Promising right now that any rules that is aliased from Makefile to build/Makefile will be logged in install.log looks too wide for me.

When logging, it is safer to log too much than too little. So I actually think this extra logging of everything going through to build/Makefile is a feature rather than a bug. Think of install.log as a log of everything regarding the build of your Sage installation. What's wrong with that? Are you afraid of losing a few megabytes of disk space?

I would have less problem if you were only adding those rules in make doc and make build

When you say make doc, do you really mean make doc or also make doc-pdf and make doc-html-jsmath and similar? When you say make build, I guess you really mean make all and make all-build (or just one of these?). You see that this becomes complicated...

comment:42 Changed 4 years ago by jdemeyer

  • Reviewers set to Nathann Cohen
  • Status changed from needs_work to needs_review

comment:43 Changed 4 years ago by git

  • Commit changed from a73b6ea46cb620432de86068d1ca58087702f69d to f3c4445950120a708b9da3bd3d2bfe68a27f09cc

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

f3c4445Move various targets to build/Makefile

comment:44 Changed 4 years ago by jdemeyer

  • Dependencies #18533, #18715, #18716 deleted

Squashed and rebased to latest beta.

comment:45 Changed 4 years ago by git

  • Commit changed from f3c4445950120a708b9da3bd3d2bfe68a27f09cc to b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc

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

b86e0bbFix strange mixture of TABs and spaces

comment:46 Changed 4 years ago by vbraun

  • Reviewers changed from Nathann Cohen to Nathann Cohen, Volker Braun
  • Status changed from needs_review to positive_review

comment:47 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/move_some_make_targets_to_build_makefile to b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:48 Changed 4 years ago by vbraun

  • Commit b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc deleted
  • Resolution fixed deleted
  • Status changed from closed to new

I'm reopening this as it totally breaks OSX, see http://build.sagedev.org/release/builders/%20%20fast%20Volker%20MiniMac%20%28OSX%2010.10%20x86_64%29%20full/builds/63/steps/compile_1/logs/stdio

I haven't tried to understand whats going on but it seems that packages don't wait for the gcc build to finish

comment:49 Changed 4 years ago by jdemeyer

Got it: the problem is that the buildbot runs make start which is indeed broken.

comment:50 Changed 4 years ago by jdemeyer

  • Branch changed from b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc to u/jdemeyer/b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc

comment:51 follow-up: Changed 4 years ago by jdemeyer

  • Commit set to 6db418e791a4d1881bd455e6814458457e904476
  • Status changed from new to needs_review

Now make start works properly after make distclean.


New commits:

f3c4445Move various targets to build/Makefile
b86e0bbFix strange mixture of TABs and spaces
1eac5fdRemove test for faux package
6db418eFix dependencies for "make start"

comment:52 in reply to: ↑ 51 Changed 4 years ago by jdemeyer

These old commits already have positive_review:

f3c4445Move various targets to build/Makefile
b86e0bbFix strange mixture of TABs and spaces

This commit was by Volker for #18788, but it really belongs on this ticket. I give it positive_review:

1eac5fdRemove test for faux package

So this is the only commit which needs to be reviewed:

6db418eFix dependencies for "make start"

comment:53 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/b86e0bb1f7f558f6f6b853a3eb0ff60a145eaebc to 6db418e791a4d1881bd455e6814458457e904476
  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:54 Changed 4 years ago by jhpalmieri

  • Commit 6db418e791a4d1881bd455e6814458457e904476 deleted

Is there a philosophy behind which targets are in the top-level Makefile and which are in build/deps? build-clean, for example? Or maybe more importantly, if someone wanted to add a new target, how to decide where it should go?

comment:55 Changed 4 years ago by vbraun

To a first approximation, I'd say stuff dealing specifically with third-party packages should go to deps and more global targets to the top-level Makefile. But then its really just putting lipstick on a pig. What we really need is a reliable build system, not a policy where to put stuff right now.

comment:56 Changed 4 years ago by jhpalmieri

What's up with

+$(STARTED): $(STANDARD_PACKES)
+	"$(SAGE_LOCAL)/bin/sage-starts"

Shouldn't that be STANDARD_PACKAGES?

comment:57 Changed 4 years ago by jdemeyer

Of course...

Note: See TracTickets for help on using tickets.