pkg() in 4.0.0-rc1 breaks building of some packages

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

pkg() in 4.0.0-rc1 breaks building of some packages

Olaf Till-2
Thought I mention it here before making a regular bug report, since it
might be release-critical.

Private function configure_make() of pkg() now calls 'make' with the
'--jobs' option, enabling parallel builds. This should only be done in
cases in which certain design rules are followed in package
Makefiles. Currently it breaks at least building the parallel package:

I've  the following lazy, stupid, and inefficient rule in this
Makefile, which of course should be changed, but which is nevertheless
legal:

%.oct: %.cc
        @MKOCTFILE@ -s -v $< sock-stream.cc

This causes sock-stream.cc to be compiled to sock-stream.o for each
built oct-file. make --jobs .. builds the oct-files in parallel, so it
happens that one job is just about to (over-)write sock-stream.o when
another job uses it for linking, causing the linking to fail.

Before I supply a (trivial) patch, it should probably be decided
whether the --jobs option should be just removed or made an option of
pkg(), defaulting to non-parallel building.

Olaf

--
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Juan Pablo Carbajal-2
On Tue, Mar 10, 2015 at 8:30 AM, Olaf Till <[hidden email]> wrote:

> Thought I mention it here before making a regular bug report, since it
> might be release-critical.
>
> Private function configure_make() of pkg() now calls 'make' with the
> '--jobs' option, enabling parallel builds. This should only be done in
> cases in which certain design rules are followed in package
> Makefiles. Currently it breaks at least building the parallel package:
>
> I've  the following lazy, stupid, and inefficient rule in this
> Makefile, which of course should be changed, but which is nevertheless
> legal:
>
> %.oct: %.cc
>         @MKOCTFILE@ -s -v $< sock-stream.cc
>
> This causes sock-stream.cc to be compiled to sock-stream.o for each
> built oct-file. make --jobs .. builds the oct-files in parallel, so it
> happens that one job is just about to (over-)write sock-stream.o when
> another job uses it for linking, causing the linking to fail.
>
> Before I supply a (trivial) patch, it should probably be decided
> whether the --jobs option should be just removed or made an option of
> pkg(), defaulting to non-parallel building.
>
Good point indeed. In the refactoring of pkg() we were working this
was meant to be an option and by defualt building is non-parallel.

> Olaf
>
> --
> public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Mike Miller-4
In reply to this post by Olaf Till-2
On Tue, Mar 10, 2015 at 08:30:34 +0100, Olaf Till wrote:

> Thought I mention it here before making a regular bug report, since it
> might be release-critical.
>
> Private function configure_make() of pkg() now calls 'make' with the
> '--jobs' option, enabling parallel builds. This should only be done in
> cases in which certain design rules are followed in package
> Makefiles. Currently it breaks at least building the parallel package:
>
> I've  the following lazy, stupid, and inefficient rule in this
> Makefile, which of course should be changed, but which is nevertheless
> legal:
>
> %.oct: %.cc
> @MKOCTFILE@ -s -v $< sock-stream.cc
>
> This causes sock-stream.cc to be compiled to sock-stream.o for each
> built oct-file. make --jobs .. builds the oct-files in parallel, so it
> happens that one job is just about to (over-)write sock-stream.o when
> another job uses it for linking, causing the linking to fail.
>
> Before I supply a (trivial) patch, it should probably be decided
> whether the --jobs option should be just removed or made an option of
> pkg(), defaulting to non-parallel building.

There should be a couple of other ways to address this without changing
pkg.

As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
shell before doing the pkg install, that should translate to --jobs=1.
So in this sense it is already an option that the end user has control
over.

In the package Makefile, you should be able to use the .NOTPARALLEL
special target to have your build ignore the --jobs setting.

HTH,

--
mike

Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Juan Pablo Carbajal-2
On Tue, Mar 10, 2015 at 2:37 PM, Mike Miller <[hidden email]> wrote:

> On Tue, Mar 10, 2015 at 08:30:34 +0100, Olaf Till wrote:
>> Thought I mention it here before making a regular bug report, since it
>> might be release-critical.
>>
>> Private function configure_make() of pkg() now calls 'make' with the
>> '--jobs' option, enabling parallel builds. This should only be done in
>> cases in which certain design rules are followed in package
>> Makefiles. Currently it breaks at least building the parallel package:
>>
>> I've  the following lazy, stupid, and inefficient rule in this
>> Makefile, which of course should be changed, but which is nevertheless
>> legal:
>>
>> %.oct: %.cc
>>       @MKOCTFILE@ -s -v $< sock-stream.cc
>>
>> This causes sock-stream.cc to be compiled to sock-stream.o for each
>> built oct-file. make --jobs .. builds the oct-files in parallel, so it
>> happens that one job is just about to (over-)write sock-stream.o when
>> another job uses it for linking, causing the linking to fail.
>>
>> Before I supply a (trivial) patch, it should probably be decided
>> whether the --jobs option should be just removed or made an option of
>> pkg(), defaulting to non-parallel building.
>
> There should be a couple of other ways to address this without changing
> pkg.
>
> As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
> shell before doing the pkg install, that should translate to --jobs=1.
> So in this sense it is already an option that the end user has control
> over.
>
This is good to know! but as you see, it is not cristal clear for
everybody. I think pkg() could be more useful if it provided a
solution...maybe just calling the line you just provided.

I personally think pkg() needs a refactoring and I was involved in it
while it lasted. I will behappy to give it another go if we believe
pkg() will be the "inside octave" tool for the user to handle
packages.

> In the package Makefile, you should be able to use the .NOTPARALLEL
> special target to have your build ignore the --jobs setting.
>
> HTH,
>
> --
> mike
>

Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Mike Miller-4
On Tue, Mar 10, 2015 at 15:19:26 +0100, Juan Pablo Carbajal wrote:
> On Tue, Mar 10, 2015 at 2:37 PM, Mike Miller <[hidden email]> wrote:
> > As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
> > shell before doing the pkg install, that should translate to --jobs=1.
> > So in this sense it is already an option that the end user has control
> > over.
> >
> This is good to know! but as you see, it is not cristal clear for
> everybody. I think pkg() could be more useful if it provided a
> solution...maybe just calling the line you just provided.

Sure, I don't think there's anything wrong with adding an option to wrap
that setting.

> I personally think pkg() needs a refactoring and I was involved in it
> while it lasted. I will behappy to give it another go if we believe
> pkg() will be the "inside octave" tool for the user to handle
> packages.

Completely agree. I remember you guys working on that project and still
hope that it makes its way back to us!

I was just pointing out that there are solutions for Olaf's problem and
that pkg probably doesn't need to be urgently changed for 4.0.

--
mike

Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Carnë Draug
In reply to this post by Juan Pablo Carbajal-2
On 10 March 2015 at 14:19, Juan Pablo Carbajal <[hidden email]> wrote:

> On Tue, Mar 10, 2015 at 2:37 PM, Mike Miller <[hidden email]> wrote:
>> On Tue, Mar 10, 2015 at 08:30:34 +0100, Olaf Till wrote:
>>> Thought I mention it here before making a regular bug report, since it
>>> might be release-critical.
>>>
>>> Private function configure_make() of pkg() now calls 'make' with the
>>> '--jobs' option, enabling parallel builds. This should only be done in
>>> cases in which certain design rules are followed in package
>>> Makefiles. Currently it breaks at least building the parallel package:
>>>
>>> I've  the following lazy, stupid, and inefficient rule in this
>>> Makefile, which of course should be changed, but which is nevertheless
>>> legal:
>>>
>>> %.oct: %.cc
>>>       @MKOCTFILE@ -s -v $< sock-stream.cc
>>>
>>> This causes sock-stream.cc to be compiled to sock-stream.o for each
>>> built oct-file. make --jobs .. builds the oct-files in parallel, so it
>>> happens that one job is just about to (over-)write sock-stream.o when
>>> another job uses it for linking, causing the linking to fail.
>>>
>>> Before I supply a (trivial) patch, it should probably be decided
>>> whether the --jobs option should be just removed or made an option of
>>> pkg(), defaulting to non-parallel building.
>>
>> There should be a couple of other ways to address this without changing
>> pkg.
>>
>> As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
>> shell before doing the pkg install, that should translate to --jobs=1.
>> So in this sense it is already an option that the end user has control
>> over.
>>
> This is good to know! but as you see, it is not cristal clear for
> everybody. I think pkg() could be more useful if it provided a
> solution...maybe just calling the line you just provided.
>
> [...]
>
>> In the package Makefile, you should be able to use the .NOTPARALLEL
>> special target to have your build ignore the --jobs setting.
>>

This is clearly a bug on the Makefile (or should make guess that it can't
be parallelized?).  Either way, I don't think we need to change anything
in Octave core.

If the only reason to default to -j1 or even to have yet another option in
pkg, is to workaround broken Makefiles, then I don't think we really need it.

The default should be to use all cores unless the user wants something else.
In such case, that choice makes sense to be applied to all computations,
not only the call to make and that option already exists.

Carnë

Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Olaf Till-2
In reply to this post by Mike Miller-4
On Tue, Mar 10, 2015 at 10:23:11AM -0400, Mike Miller wrote:

> On Tue, Mar 10, 2015 at 15:19:26 +0100, Juan Pablo Carbajal wrote:
> > On Tue, Mar 10, 2015 at 2:37 PM, Mike Miller <[hidden email]> wrote:
> > > As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
> > > shell before doing the pkg install, that should translate to --jobs=1.
> > > So in this sense it is already an option that the end user has control
> > > over.
> > >
> > This is good to know! but as you see, it is not cristal clear for
> > everybody. I think pkg() could be more useful if it provided a
> > solution...maybe just calling the line you just provided.
>
> Sure, I don't think there's anything wrong with adding an option to wrap
> that setting.
>
> > I personally think pkg() needs a refactoring and I was involved in it
> > while it lasted. I will behappy to give it another go if we believe
> > pkg() will be the "inside octave" tool for the user to handle
> > packages.
>
> Completely agree. I remember you guys working on that project and still
> hope that it makes its way back to us!
>
> I was just pointing out that there are solutions for Olaf's problem and
> that pkg probably doesn't need to be urgently changed for 4.0.
For the record, I didn't post this because it's "my problem"; the
Makefile is already changed, and a decent Makefile probably shouldn't
have this problem. The point is that the _default_ should be "no
parallel builds", since pkg() can be used to compile any package, not
only those of OF.

My suggestion is to just drop the --jobs option unconditionally, this
change is small, a few lines to be deleted. Most packages are not so
large that parallel building is a great advantage, so it's barely
worth to have an additional pkg option for it -- IMHO.

Just my 2 cents.

Olaf

--
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkg() in 4.0.0-rc1 breaks building of some packages

Carnë Draug
On 10 March 2015 at 15:47, Olaf Till <[hidden email]> wrote:

> On Tue, Mar 10, 2015 at 10:23:11AM -0400, Mike Miller wrote:
>> On Tue, Mar 10, 2015 at 15:19:26 +0100, Juan Pablo Carbajal wrote:
>> > On Tue, Mar 10, 2015 at 2:37 PM, Mike Miller <[hidden email]> wrote:
>> > > As the end user, you can call `setenv OMP_NUM_THREADS 1` in the Octave
>> > > shell before doing the pkg install, that should translate to --jobs=1.
>> > > So in this sense it is already an option that the end user has control
>> > > over.
>> > >
>> > This is good to know! but as you see, it is not cristal clear for
>> > everybody. I think pkg() could be more useful if it provided a
>> > solution...maybe just calling the line you just provided.
>>
>> Sure, I don't think there's anything wrong with adding an option to wrap
>> that setting.
>>
>> > I personally think pkg() needs a refactoring and I was involved in it
>> > while it lasted. I will behappy to give it another go if we believe
>> > pkg() will be the "inside octave" tool for the user to handle
>> > packages.
>>
>> Completely agree. I remember you guys working on that project and still
>> hope that it makes its way back to us!
>>
>> I was just pointing out that there are solutions for Olaf's problem and
>> that pkg probably doesn't need to be urgently changed for 4.0.
>
> For the record, I didn't post this because it's "my problem"; the
> Makefile is already changed, and a decent Makefile probably shouldn't
> have this problem. The point is that the _default_ should be "no
> parallel builds", since pkg() can be used to compile any package, not
> only those of OF.

Parallel builds will work unless the Makefile is not correctly written,
in which the problem is on the package, not on Octave.  pkg() can install
any package, even using parallel builds.  Provided the package is
correctly written.  That has nothing to do with OF.

> My suggestion is to just drop the --jobs option unconditionally, this
> change is small, a few lines to be deleted. Most packages are not so
> large that parallel building is a great advantage, so it's barely
> worth to have an additional pkg option for it -- IMHO.

The whole reason why this got implemented was because users complained that
building packages was taking too long [1].  Should we slow it down again,
when the only advantage is to allow installation of packages with
buggy Makefiles?  I really don't think so.  Or is there any other
advantage?

Carnë

[1] https://savannah.gnu.org/bugs/?42915