[PATCH] Fix Parallel Building #15593
Comments
From @tomhukinsCreated by @tomhukinsCurrently, I see parallel builds fail every time on FreeBSD. The Perl Info |
From @tomhukins0001-Fix-parallel-building.patchFrom 309830d1990f6125d1fed459cf15f8e424b7dfab Mon Sep 17 00:00:00 2001
From: Tom Hukins <tom@eborcom.com>
Date: Thu, 8 Sep 2016 16:01:36 +0000
Subject: [PATCH] Fix parallel building
Commit 3dfcef7e97bf1b516f added a dependency on Porting/pod_lib.pl to
Porting/manisort. Porting/pod_lib.pl depends on File::Find which in
turn depends on Cwd.
Stating Cwd as a requirement ensures the $(MANIFEST_SRT) target builds
successfully with "make -jX" where X > 1. Without this fix, I see this
target fail every time because perl can't load Cwd.
---
Makefile.SH | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile.SH b/Makefile.SH
index 42beb81..27f1e66 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -600,7 +600,7 @@ all: $(FIRSTMAKEFILE) $(MINIPERL_EXE) $(generated_pods) $(private) $(unidatafile
@echo " ";
@echo " Everything is up to date. Type '$(MAKE) test' to run test suite."
-$(MANIFEST_SRT): MANIFEST $(PERL_EXE)
+$(MANIFEST_SRT): MANIFEST $(PERL_EXE) lib/auto/Cwd/Cwd$(DLSUFFIX)
@$(RUN_PERL) Porting/manisort -q || (echo "WARNING: re-sorting MANIFEST"; \
$(RUN_PERL) Porting/manisort -q -o MANIFEST; sh -c true)
@touch $(MANIFEST_SRT)
--
2.9.2
|
From @jkeenanOn Thu Sep 08 09:05:09 2016, tomhukins wrote:
I'm a bit puzzled by this report. I have been smoke-testing blead on FreeBSD approximately twice a week for the past month, typically with '-j8', and I have yet to see evidence of this problem. Could you provide more evidence of what you're seeing? Also, AFAICT there was nothing FreeBSD-specific in commit 3dfcef7. Nor is there anything FreeBSD-specific in your patch. 3dfcef7 was committed to blead on June 21. Hence, if there were a problem with parallel builds I would have expected it to show up on multiple OSes, not just FreeBSD, and to have started to appear three months ago. Can you clarify? Thank you very much. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tomhukinsOn Sun, Sep 18, 2016 at 04:52:00PM -0700, James E Keenan via RT wrote:
In case I was unclear, I wouldn't expect a dependency error to always
Here's the end of the output I see when running Processing PropertyAliases.txt
I agree with both your statements.
Yes, I don't mean to suggest that the build will fail every time time in I noticed that setting "-Doptimize='-O2'" makes the problem go away in As I wrote in my commit message, I believe the cause of these symptoms Tom |
From @LeontOn Mon, Sep 19, 2016 at 1:52 AM, James E Keenan via RT <
Which make do you use? Gnu make or BSD make? Same question to Tom Leon |
From @tomhukinsOn Mon, Sep 19, 2016 at 04:10:59PM +0200, Leon Timmermans wrote:
I'm using the default BSD make that ships with FreeBSD 9.3. Tom |
From @jkeenanOn Mon Sep 19 06:50:57 2016, tomhukins wrote:
Aha! You are not alone! I reported this (albeit, without much detail or insight) on list a little over two weeks ago: http://www.nntp.perl.org/group/perl.perl5.porters/2016/09/msg239501.html And because the errors were only intermittent, I forgot about them. And the OS in question was FreeBSD 10.3; I don't see this on Ubuntu Linux 16.04 LTS. Replying to Leon: The version of make which I use there is whatever the default version of make is on FreeBSD. There doesn't appear to be a --version switch for make, but 'man make' contains the following: #####
I will try to build some perls on FreeBSD and see if I can capture error output like yours. Thank you very much. -- |
From @jkeenanOn Mon Sep 19 09:33:14 2016, jkeenan wrote:
Well, I've now on my 43rd build of blead on FreeBSD 10.3 -- and have yet to get a build failure! In any event, I agree with the logic in Tom's patch and believe it is worth trying out. I have pushed it to this smoke-testing branch: smoke-me/jkeenan/129229-cwd-dependency Please try this out on multiple OSes, as the fix will apply to all platforms, not just FreeBSD. Thank you very much. -- |
From @kentfredricOn 19 September 2016 at 11:52, James E Keenan via RT
Its been showing up since at least 5.24.0 for me on linux ( That is, ( Granted whether or not we need to patch the manifest is a separate If there were other things that happened which caused the manifest to 3dfcef7 just appears to have shuffled the deck enough that -- KENTNL - https://metacpan.org/author/KENTNL |
From @cpansproutOn Mon Sep 19 23:50:58 2016, kentfredric@gmail.com wrote:
What I want to know is why the build is trying to sort MANIFEST at all. Ideally, ‘make’ should *never* touch checked-in files, because then ‘make distclean’ doesn’t work. You can’t get a clean slate unless you are using a git repository. -- Father Chrysostomos |
From @cpansproutOn Tue Sep 20 00:36:46 2016, sprout wrote:
Does the attached diff also fix the problem? (Instead of working around the problem, it removes the cause.) -- Father Chrysostomos |
From @cpansproutInline Patchdiff --git a/Makefile.SH b/Makefile.SH
index 42beb81..8007e43 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -596,7 +596,7 @@ splintfiles = $(c1)
@echo `$(CCCMDSRC)` -S $*.c
@`$(CCCMDSRC)` -S $*.c
-all: $(FIRSTMAKEFILE) $(MINIPERL_EXE) $(generated_pods) $(private) $(unidatafiles) $(public) $(dynamic_ext) $(nonxs_ext) extras.make $(MANIFEST_SRT)
+all: $(FIRSTMAKEFILE) $(MINIPERL_EXE) $(generated_pods) $(private) $(unidatafiles) $(public) $(dynamic_ext) $(nonxs_ext) extras.make
@echo " ";
@echo " Everything is up to date. Type '$(MAKE) test' to run test suite."
|
From @cpansproutOn Tue Sep 20 00:40:13 2016, sprout wrote:
Digging a little further, I see this commit: commit 19bf100 automatically sort the MANIFEST if necessary The problem with automatically sorting the MANIFEST is what I mentioned above: ‘make’ should not make changes to files tracked by git. I think that commit was ill-advised. -- Father Chrysostomos |
From @tomhukinsOn Tue, Sep 20, 2016 at 12:40:13AM -0700, Father Chrysostomos via RT wrote:
Yes, I ran "make -j4" three times with this patch and saw no failures. Tom |
From @kentfredricOn 20 September 2016 at 19:43, Father Chrysostomos via RT
Before that commit, the patching the manifest causes a handful of And given we're patching, compiling, testing, and then installing on But I'll have to see if the fix works ( hard to know for sure because I suspect it will, because I'd have suggested a similar patch. -- KENTNL - https://metacpan.org/author/KENTNL |
From @iabynOn Tue, Sep 20, 2016 at 10:59:42PM +1200, Kent Fredric wrote:
How about we just don't bother keeping MANIFEST strictly sorted during That does however leave open the issue of whether we should still -- |
From @cpansproutOn Tue Sep 20 05:07:29 2016, davem wrote:
That makes sense. I was thinking of proposing that next if anyone objected to a simple revert of the change. I have been wondering since I started hacking on perl why we needed to keep it sorted anyway. The best way to solve something that shouldn’t be a problem to begin with is to remove the problem instead of working around it.
manifest.t should continue to perform its other tests. -- Father Chrysostomos |
From @nawglanOn Tue Sep 20 05:59:20 2016, sprout wrote:
Can the MANIFEST file be sorted via a git hook? -- |
From @dcollinsnIf MANIFEST is procedurally generated, is there such a thing as a -- On Tue, Sep 20, 2016 at 8:59 AM, Father Chrysostomos via RT <
|
From @demerphqOn 20 Sep 2016 8:59 a.m., "Father Chrysostomos via RT" <
I object to your change so long as we test for sorted manifests routinely. As the comment shows I only made the change to eliminate manual build steps
That assumes there is consensus about what the problem is. Historically So when this issue came up for me I did what hackers always do and
I think manifest.t shouldn't be an automated test. We should have a make Yves |
From @demerphqOn 20 Sep 2016 3:43 a.m., "Father Chrysostomos via RT" <
I think we have to differ on that one. The only way a patch ends up And I don't agree about your make clean argument either in your other post That make might correct an innocuous error you made in a source file while Anyway my view is that if we routinely test for something that can be fixed Yves |
From @cpansproutOn Tue Sep 20 07:26:04 2016, demerphq wrote:
1. Make changes I follow that routine routinely. If I get ‘Can’t rebase because you have unstaged changes’ when I *know* I have committed, I am going to get very frustrated. :-)
But what if I haven’t made any changes, but simply tried to build perl?
No, instead of doing that command automatically, have a ‘make porting’ target that gets the tests passing, so that the hacker has more control over when things get run. For non-critical things like manifest sorting, we can remove the tests and have ‘make porting’ as a standard thing to run when making a release. (If people want to run ‘make porting’ from time to time themselves, they can.)
I hope my suggestions will do the same, but without causing problems. -- Father Chrysostomos |
From @demerphqOn 20 Sep 2016 12:51, "Father Chrysostomos via RT" <
Not as frustrated as I get when I make a change and it fails test because I mean has it happened even once yet? Also personally I think committing before you run make test is a bit
Lots of what ifs here, but have any of them happened? I mean basically you are saying I should be frustrated about something so Luckily there is an option where neither of us have to be frustrated.
I am fully supportive of this option, and for more than just manifest.t
I do too. Yves |
From @demerphqOn 21 Sep 2016 09:53, "demerphq" <demerphq@gmail.com> wrote:
By the way there is an option available that we did not discuss. We could That would make both of us happy right? In my workflow the test would not Yves |
From @cpansproutOn Wed Sep 21 06:54:38 2016, demerphq wrote:
I used not to, but the tests that make sure that files are actually listed in MANIFEST would blithely pass, and then I would commit and push and get smoke failures. This happened again, and again, and again, so I changed my workflow. -- Father Chrysostomos |
From @cpansproutOn Wed Sep 21 07:00:45 2016, demerphq wrote:
Would it not still cause the problem that was the subject of this ticket to begin with? It seems to me that we are using fragile fixes to fix fragile fixes when we could just ‘not do that’ to begin with.
-- Father Chrysostomos |
From @bulk88On Tue Sep 20 00:43:37 2016, sprout wrote:
I think so too. I never submit a patch to p5p without a "make test_porting" first (1-2 minutes for all the tests, excluding build time) which covers the manifest sorting. That patch sounds like its CYA for commiters who think "this can't possibly fail tests so why waste my time running any tests or regen.pl?" I'd rather not see the makefile dependency tree made into more of a ball of yarn that limits build speed on high core count machine (unable to max out all cores). -- |
From @ap* Dave Mitchell <davem@iabyn.com> [2016-09-20 14:12]:
I had the same thought to begin with. Then I thought on it a little further, and ended up disagreeing. If we want MANIFEST to be sorted at all, then we should make sure that But writing that down served as my talk-to-the-teddybear: it reminded me But if we’re writing tooling anyway, it would not be hard to also write Then we should move the test for proper sorting to the release process, The alternative to all this is to say we don’t care about the order in
That should stay. There should maybe even be a hook for it. * Father Chrysostomos via RT <perlbug-followup@perl.org> [2016-09-21 18:36]:
It seems not a fragile fix to me but exactly the right idea. Though it That would not address people who run tests prior to committing, but if * Desmond Daignault via RT <perlbug-followup@perl.org> [2016-09-21 01:48]:
:-) Regards, |
From @kentfredricOn 23 September 2016 at 18:18, Aristotle Pagaltzis <pagaltzis@gmx.de> wrote:
There's another option .... server side push hooks that assert This at least means people who don't have the magic locally can't push Ideally there should also be a way to make it force, in the event -- KENTNL - https://metacpan.org/author/KENTNL |
From @tonycozOn Thu Sep 08 09:05:09 2016, tomhukins wrote:
In general this type of problem is easy to reproduce once you identify $ ./Configure -des -Dusedevel && make -j6 MANIFEST.srt Whether or not we should have a MANIFEST file, or if we have the file Something like the attached fixes that. Tony |
From @tonycoz0001-perl-129229-fix-dependency-for-MANIFEST_SRT.patchFrom b635474a1457d7bc466fed8d0223210a6d873686 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 12 Oct 2016 09:56:43 +1100
Subject: (perl #129229) fix dependency for $(MANIFEST_SRT)
Correctly handles -Uusedl and -Dstatic_ext=Cwd builds
---
Makefile.SH | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/Makefile.SH b/Makefile.SH
index 511d6e3..487231d 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -461,9 +461,20 @@ SH_to_target() {
SH='Makefile.SH cflags.SH config_h.SH makedepend.SH myconfig.SH runtests.SH pod/Makefile.SH'
shextract=`SH_to_target $SH`
+case "$usedl$static_cwd" in
+defineundef)
+ manifest_deps="lib/auto/Cwd/Cwd\$(DLSUFFIX)"
+ ;;
+*)
+ manifest_deps=
+ ;;
+esac
+
## In the following dollars and backticks do not need the extra backslash.
$spitshell >>$Makefile <<!GROK!THIS!
+manifest_deps = $manifest_deps
+
private = preplibrary \$(CONFIGPM) \$(CONFIGPOD) git_version.h lib/buildcustomize.pl
# Files to be built with variable substitution before miniperl
@@ -600,7 +611,7 @@ all: $(FIRSTMAKEFILE) $(MINIPERL_EXE) $(generated_pods) $(private) $(unidatafile
@echo " ";
@echo " Everything is up to date. Type '$(MAKE) test' to run test suite."
-$(MANIFEST_SRT): MANIFEST $(PERL_EXE)
+$(MANIFEST_SRT): MANIFEST $(PERL_EXE) $(manifest_deps)
@$(RUN_PERL) Porting/manisort -q || (echo "WARNING: re-sorting MANIFEST"; \
$(RUN_PERL) Porting/manisort -q -o MANIFEST; sh -c true)
@touch $(MANIFEST_SRT)
--
2.1.4
|
From @arcTony Cook via RT <perlbug-followup@perl.org> wrote:
Thanks, that's a very useful tip.
I started wondering why Porting/manisort needs Cwd at all. It turns So an alternative would be to restructure Porting/pod_lib.pl: either See attached. -- |
From @arc0001-RT-129229-move-sort_manifest-into-its-own-library.patchFrom 0cd563745f7dff93ad04af9f9117623474a42a4e Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
Date: Wed, 12 Oct 2016 10:34:13 +0100
Subject: [PATCH] RT#129229: move sort_manifest() into its own library
This means that the MANIFEST.srt target in the Makefile no longer needs
to load a library that depends on Cwd (and other potentially-dynamic
modules). That in turn fixes a missing-dependency bug in the Makefile.
---
MANIFEST | 1 +
Porting/README.pod | 4 ++++
Porting/manifest_lib.pl | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
Porting/manisort | 2 +-
Porting/pod_lib.pl | 29 ---------------------------
Porting/pod_rules.pl | 1 +
6 files changed, 60 insertions(+), 30 deletions(-)
create mode 100644 Porting/manifest_lib.pl
diff --git a/MANIFEST b/MANIFEST
index 065ca84..f56a336 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -4998,6 +4998,7 @@ Porting/make_snapshot.pl Make a tgz snapshot of our tree with a .patch file in i
Porting/makemeta Create the top-level META.yml
Porting/makerel Release making utility
Porting/manicheck Check against MANIFEST
+Porting/manifest_lib.pl Library for checking and sorting the MANIFEST
Porting/manisort Sort the MANIFEST
Porting/new-perldelta.pl Generate a new perldelta
Porting/newtests-perldelta.pl Generate Perldelta stub for newly added tests
diff --git a/Porting/README.pod b/Porting/README.pod
index 21a0414..c00096e 100644
--- a/Porting/README.pod
+++ b/Porting/README.pod
@@ -245,6 +245,10 @@ web page to use to generate the snapshot files.
This script outputs a list of files in F<MANIFEST> which don't exist and a
list of files that exist and aren't in F<MANIFEST>.
+=head2 F<manifest_lib.pl>
+
+This library provides functions used in checking and sorting the F<MANIFEST>.
+
=head2 F<manisort>
This script sorts the files in F<MANIFEST>.
diff --git a/Porting/manifest_lib.pl b/Porting/manifest_lib.pl
new file mode 100644
index 0000000..0b63046
--- /dev/null
+++ b/Porting/manifest_lib.pl
@@ -0,0 +1,53 @@
+#!/usr/bin/perl
+
+use strict;
+
+=head1 NAME
+
+Porting/manifest_lib.pl - functions for managing manifests
+
+=head1 SYNOPSIS
+
+ require './Porting/manifest_lib.pl';
+
+=head1 DESCRIPTION
+
+=head2 C<sort_manifest>
+
+Treats its arguments as (chomped) lines from a MANIFEST file, and returns that
+listed sorted appropriately.
+
+=cut
+
+# Try to get a sane sort. case insensitive, more or less
+# sorted such that path components are compared independently,
+# and so that lib/Foo/Bar sorts before lib/Foo-Alpha/Baz
+# and so that lib/Foo/Bar.pm sorts before lib/Foo/Bar/Alpha.pm
+# and so that configure and Configure sort together.
+sub sort_manifest {
+ return
+ # case insensitive sorting of directory components independently.
+ map { $_->[0] } # extract the full line
+ sort {
+ $a->[1] cmp $b->[1] || # sort in order of munged filename
+ $a->[0] cmp $b->[0] # then by the exact text in full line
+ }
+ map {
+ # split out the filename and the description
+ my ($f) = split /\s+/, $_, 2;
+ # lc the filename so Configure and configure sort together in the list
+ my $m= lc $f; # $m for munged
+ # replace slashes by nulls, this makes short directory names sort before
+ # longer ones, such as "foo/" sorting before "foo-bar/"
+ $m =~ s!/!\0!g;
+ # replace the extension (only one) by null null extension.
+ # this puts any foo/blah.ext before any files in foo/blah/
+ $m =~ s!(\.[^.]+\z)!\0\0$1!;
+ # return the original string, and the munged filename
+ [ $_, $m ];
+ } @_;
+}
+
+1;
+
+# ex: set ts=8 sts=4 sw=4 et:
diff --git a/Porting/manisort b/Porting/manisort
index 72cbb9c..3d698e2 100644
--- a/Porting/manisort
+++ b/Porting/manisort
@@ -14,7 +14,7 @@ $| = 1;
# Get command line options
use Getopt::Long;
-require "Porting/pod_lib.pl";
+require "Porting/manifest_lib.pl";
my $outfile;
my $check_only = 0;
my $quiet = 0;
diff --git a/Porting/pod_lib.pl b/Porting/pod_lib.pl
index 4ad204c..6eaacde 100644
--- a/Porting/pod_lib.pl
+++ b/Porting/pod_lib.pl
@@ -663,35 +663,6 @@ sub get_pod_metadata {
return \%state;
}
-# Try to get a sane sort. case insensitive, more or less
-# sorted such that path components are compared independently,
-# and so that lib/Foo/Bar sorts before lib/Foo-Alpha/Baz
-# and so that lib/Foo/Bar.pm sorts before lib/Foo/Bar/Alpha.pm
-# and so that configure and Configure sort together.
-sub sort_manifest {
- return
- # case insensitive sorting of directory components independently.
- map { $_->[0] } # extract the full line
- sort {
- $a->[1] cmp $b->[1] || # sort in order of munged filename
- $a->[0] cmp $b->[0] # then by the exact text in full line
- }
- map {
- # split out the filename and the description
- my ($f) = split /\s+/, $_, 2;
- # lc the filename so Configure and configure sort together in the list
- my $m= lc $f; # $m for munged
- # replace slashes by nulls, this makes short directory names sort before
- # longer ones, such as "foo/" sorting before "foo-bar/"
- $m =~ s!/!\0!g;
- # replace the extension (only one) by null null extension.
- # this puts any foo/blah.ext before any files in foo/blah/
- $m =~ s!(\.[^.]+\z)!\0\0$1!;
- # return the original string, and the munged filename
- [ $_, $m ];
- } @_;
-}
-
1;
# ex: set ts=8 sts=4 sw=4 et:
diff --git a/Porting/pod_rules.pl b/Porting/pod_rules.pl
index af5550e..37cbb9c 100644
--- a/Porting/pod_rules.pl
+++ b/Porting/pod_rules.pl
@@ -33,6 +33,7 @@ if (ord("A") == 193) {
);
require 'Porting/pod_lib.pl';
+require 'Porting/manifest_lib.pl';
sub my_die;
# process command-line switches
--
2.7.4
|
From @jkeenanOn Wed Oct 12 03:04:01 2016, arc wrote:
In order to move the discussion forward, I've created a smoke-testing branch for Aaron Crane's latest patch: smoke-me/jkeenan/arc/129229-manifest-lib Since I've gotten the 'make' failure quite a few times on my FreeBSD VMs, I'm motivated to get this straightened out. I'm attaching an excerpt from a build failure today on FreeBSD-11.0. Thank you very much. -- |
From @jkeenancc -o perl -pthread -Wl,-E -fstack-protector-strong -L/usr/local/lib perlmain.o libperl.a `cat ext.libs` -lpthread -lm -lcrypt -lutil make: stopped in /usr/home/jkeenan/gitwork/perl make: stopped in /usr/home/jkeenan/gitwork/perl |
From @tonycozOn Wed Oct 12 03:04:01 2016, arc wrote:
Inline Patchdiff --git a/Porting/manifest_lib.pl b/Porting/manifest_lib.pl
new file mode 100644
index 0000000..0b63046
--- /dev/null
+++ b/Porting/manifest_lib.pl
@@ -0,0 +1,53 @@
+#!/usr/bin/perl
+
(I don't know why Nicholas added one to pod_lib.pl when he created it either.) Tony |
From @jkeenanOn Sat Oct 22 17:32:17 2016, jkeenan wrote:
I'm attaching a different instance of what I suspect is the same kind of build failure, again from FreeBSD (though I don't recall whether this was 10.3 or 11.0). Thank you very much. -- |
From @jkeenanlib/unicore/mktables: Files seem to be ok, not bothering to rebuild. Add '-w' option to force build make: stopped in /usr/home/jkeenan/gitwork/perl make: stopped in /usr/home/jkeenan/gitwork/perl |
From @tomhukinsOn Mon, Oct 24, 2016 at 08:24:54AM -0700, James E Keenan via RT wrote:
Yes, this looks like the same problem to me.
The MANIFEST.srt make target fails to find Cwd.pm. The original patch I Tom |
From @demerphqOn 24 October 2016 at 17:35, Tom Hukins <tom@eborcom.com> wrote:
I will merge James' patch in a few minutes if it passes test. Which The arguments from FC are a bit different and should be dealt with in Yves -- |
From @demerphqOn 24 October 2016 at 17:42, demerphq <demerphq@gmail.com> wrote:
So I have done this. Yves -- |
From @demerphqOn 24 October 2016 at 17:57, demerphq <demerphq@gmail.com> wrote:
To dispel some confusion: I merged the following: commit 4a59181 RT#129229: move sort_manifest() into its own library This means that the MANIFEST.srt target in the Makefile no longer needs -- |
From @tomhukinsOn Mon, Oct 24, 2016 at 05:42:01PM +0200, demerphq wrote:
The applied patch has fixed the problem I encountered. If the Thank you all, |
|
@xsawyerx - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#129229 (status was 'resolved')
Searchable as RT129229$
The text was updated successfully, but these errors were encountered: