[m-rev.] For review: Pull request 4.

[Julien Fischer]

On Wed, Jan 9, 2013 at 3:23 PM, Paul Bone <paul at bone.id.au> wrote:
> On Wed, Jan 09, 2013 at 12:50:24AM +1100, Julien Fischer wrote:
>> On Wed, Jan 9, 2013 at 12:40 AM, Paul Bone <paul at bone.id.au> wrote:
>> >
>> > Nikolay Orlyuk has submitted these four patches for review via github.
>> >
>> > The first three are concerned with installation, paths and the DESTDIR
>> > variable.  The forth fixes an issue with parallel builds.
>> >
>> > I'm happy to review the first three, as I introduced the DESTDIR variable.
>> > Can someone review the last one?
>>
>> See my comments in the bug database re: bug #273.  The reason for copying
>> the files from the mdcomp directory rather than just linking them normally is
>> because in debugging grades the versions in the slice and deep
>> profiling directories
>> need to be compiled differently.
>>
>> The problem here is caused by attempting to build the compiler from
>> scratch by doing
>>
>>     $ make PARALLEL=-jN
>>
>> The parallel build should work without problems if you do
>>
>>    $ mmake depend
>>    $ mmake -jN
>>
>> instead, since it is the depend target that responsible for copying the files
>> from the mdbcomp directory.  (This is of course precisely what INSTALL{git,_CVS)
>> says you should do**.)
>>
>> Invoking make instead of mmake should really be reserved for compiling
>> the source
>> distribution IMO.  The problem obviously won't exist for the source
>> dist. since all the files have
>> already been copied.
>>
>> In short, this change would break our ability to test some grades and
>> can be worked around
>> anyway.
>>
>
> It turns out there's a really simple fix for the race condition, and it
> keeps the structure of the makefiles the same.  I will prepare a full patch for
> this.
>
> diff --git a/deep_profiler/Mmakefile b/deep_profiler/Mmakefile
> index c43fc09..703c4f9 100644
> --- a/deep_profiler/Mmakefile
> +++ b/deep_profiler/Mmakefile
> @@ -121,17 +121,25 @@ Mercury.modules: DEEP_FLAGS
>  .PHONY: all
>  all:   $(MDBCOMP_MODULES) $(ALL_DEEP_MODULES) $(TAGS_FILE_EXISTS)
>
> +# We use the sentinel file .mdbcomp_modules to avoid the race condition that
> +# exists because the rule would normally be invoked for each of the mdbcomp
> +# modules.  However, we also need to use the no-op action '@' to allow make
> +# to see that the timestamps on the $(MDBCOMP_MODULES) have changed and that
> +# other things may need rebuilding.
> +$(MDBCOMP_MODULES): .mdbcomp_modules
> +       @
> +
>  # We need to start by turning write permission on for each copied file
>  # in case some exist, but we need to ignore errors in case some don't exist.
>  # The exit 0 is to prevent make itself from printing a message about the
>  # (ignored) failure of an action.
>  #
>  # We could modify the action here to copy only the changed files.
> -
> -$(MDBCOMP_MODULES): $(MDBCOMP_ORIG_MODULES)
> +.mdbcomp_modules : $(MDBCOMP_ORIG_MODULES)
>         - at chmod a+w $(MDBCOMP_MODULES) > /dev/null 2>&1; exit 0
>         cp $(MDBCOMP_ORIG_MODULES) .
>         @chmod a-w $(MDBCOMP_MODULES)
> +       touch $@
>
>  #-----------------------------------------------------------------------------#

Ok, that addresses the problem I had with the original patch.

Cheers,
Julien.