distcc for ada (part 1 of 2)

[Tim Dossett-2]

Hello all,
I have modified distcc 3.1 to distribute ada compilation as well as c
and c++, and I'd like to contribute the modifications back to the
project. Patch is attached. Please consider this for inclusion in the distcc baseline.

I had to split the patch into 2 parts so it would not bounce. To create the patch, run:

cat distcc-3.1-ada-0.3.patch.split_aa  distcc-3.1-ada-0.3.patch.split_ab > distcc-3.1-ada-0.3.patch

Thanks,
Tim

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Fergus Henderson-4]

On Wed, Oct 14, 2009 at 4:25 PM, Tim Dossett <timothy.dossett@...> wrote:

Hello all,
I have modified distcc 3.1 to distribute ada compilation as well as c
and c++, and I'd like to contribute the modifications back to the
project. Patch is attached. Please consider this for inclusion in the distcc baseline.

Cool, that looks great - thanks for the patch!
It is especially nice to see how the development of pump mode has helped you to add support for Ada.

Here's some comments from a fairly quick initial scan:

- If possible, could you please base the patch on the current sources in SVN, rather than on the last release?
- The changes to configure.ac and configure should be reverted.
- For the NEWS file, could you condense the new entries into a single one?
- Are you sure that we handle "-x c", "-x c++", and "-x objective-c" correctly?  It might be better to fall back to local execution for those.
- What happens if you mix file extensions with "-x" options, e.g. "distcc -c -x ada foo.c" or "distcc -c -x c foo.ada"?
  I suspect that probably doesn't work...
- The change to max_discrepancies_before_demotion is not the right thing to do for C/C++.
- Don't hard-code the port number to use during tests (especially not to 3633); try a port, and if that doesn't work, keep trying higher port numbers
- Some documentation of the new protocol in doc/protocol-4.txt would be nice.
- Do we need a new host option (similar to ",lzo" or ",lzo,cpp") for version 4 of the protocol?
- It would be nice if the Ada tests could be written in such a way that if the required compilers aren't installed they pass, but print a warning message (similar to the NOTRUN messages printed by the current tests), and perhaps integrated into "make check".  Or alternatively, add some simple tests of Ada functionality to test/testdistcc.py.
- It would be nice to have at least one test of a _failing_ Ada compilation.
- Some unit tests for include_server/Ada.py would be nice.

If you could update the patch after considering these comments, that would be great.

Cheers,
  Fergus.

--
Fergus Henderson <fergus@...>

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Tim Dossett-2]

Thanks for the comments.

I'll re-baseline to the current SVN; might take a week or so since this is a spare time job for me.

Since the -x option only explicitly tells gcc what the source is supposed to be, shouldn't distcc already support -x c, -x c++ and -x objective-c? The reason I added code to support -x is the ada multi-language build tool "gprbuild" by default adds -x for all languages it supports. Haven't tried mixing the -x option with the wrong source type.

Agree that max_discrepancies_before_demotion = 10000000 (or whatever large number) is wrong for c/c++ and ada too, but 1 seems too strict. I think 10 sounds more reasonable, allowing some remote compile failures without reverting to non-pump compiles. It would be nice to make this user configurable without recompiling.

Agree about the test comments; I didn't spend enough time trying to understand the current test approach, and how to integrate with it. I'll take another look.

Need something to distinguish protocol version 4, but personally, I think a server version query would be preferable to another suffix on the hostname. I didn't update the server version query response for version 4; this is a bug.

I'll write something up in doc/protocol-4.txt for the protocol version 4 changes.

Thanks again,
Tim

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Fergus Henderson-4]

On Thu, Oct 15, 2009 at 12:14 PM, Tim Dossett <timothy.dossett@...> wrote:

Thanks for the comments.

I'll re-baseline to the current SVN; might take a week or so since this is a spare time job for me.

Since the -x option only explicitly tells gcc what the source is supposed to be, shouldn't distcc already support -x c, -x c++ and -x objective-c?

I think distcc has some code that makes assumptions about the language based on the file extension.
That code would need to change in order to properly support "-x".

The reason I added code to support -x is the ada multi-language build tool "gprbuild" by default adds -x for all languages it supports. Haven't tried mixing the -x option with the wrong source type.

Maybe you could change the distcc code so that distcc supports "-x" only in the cases where the "-x" option has no effect?
That might be easier than supporting it properly, and might be sufficient for use with "gprbuild".

Agree that max_discrepancies_before_demotion = 10000000 (or whatever large number) is wrong for c/c++ and ada too, but 1 seems too strict. I think 10 sounds more reasonable, allowing some remote compile failures without reverting to non-pump compiles. It would be nice to make this user configurable without recompiling.

I agree and I would be very happy to accept a patch along these lines.
 

Cheers,
  Fergus.

--
Fergus Henderson <fergus@...>

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Tim Dossett-2]

I've re-baselined the ada patch to SVN r709, and incorporated most of
your comments, with some work still to be done in the testing area;
however, I wanted to make this updated patch available for review and
comment.

To apply the patch:

cd <distcc_version_709_directory>
patch -p0 < ../V709_ada.patch





__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc