Closed GoogleCodeExporter closed 9 years ago
I suggest you go ahead and make a patch -- I don't know of any other solution.
Original comment by fer...@google.com
on 19 Sep 2011 at 12:12
Attached is the patch for the parse_command.py for the pump server.
The problem that remains is that I now get this error:
distcc[20879] ERROR: compile example_util.cpp on gold:3363/4,lzo,cpp failed
distcc[20879] (dcc_build_somewhere) Warning: remote compilation of
'example_util.cpp' failed, retrying locally
distcc[20879] Warning: failed to distribute example_util.cpp to
gold:3363/4,lzo,cpp, running locally instead
distcc[20879] (dcc_please_send_email_after_investigation) Warning: remote
compilation of 'example_util.cpp' failed, retried locally and got a different
result.
distcc[20879] (dcc_note_discrepancy) Warning: now using plain distcc, possibly
due to inconsistent file system changes during build
I assume that the compile.c/remote.c needs to be changed to expand the response
files as well in either of the functions:
dcc_compile_remote
dcc_compile_local
dcc_build_somewhere_timed
dcc_build_somewhere
which all take *argv[] - which should be expanded as well.
Can you advice what the best place for the expansion would be (I might have
even overlooked functions that depend on the arguments? Or would the function,
that adds the compiler name to the list of arguments be the best place?)
Thanks
Original comment by nilswoet...@gmail.com
on 20 Sep 2011 at 10:19
Attachments:
I would be inclined to start at main() and traverse down through the call graph
until you get to a function that passes argc/argv to more than one subroutine,
or that manipulates argc/argv to parse the command line. That function (which
could be main() itself) is likely to be the best place to expand response files.
Original comment by fergus.h...@gmail.com
on 7 Feb 2012 at 1:49
The attached patch adds a function that expands response files recursively (up
to depth 10); it combines the previous patch for the parse_command.py for the
include_server component and adds the fixes that were necessary in the C
sources (creating the "compile_flags" argument in distcc.c main() function.
I tried to stick to the coding conventions.
Hope it finds approval and will be part of a new release.
Original comment by nilswoet...@gmail.com
on 29 Feb 2012 at 7:37
Attachments:
The patch has potential buffer overflows, e.g. using fscanf( fp, "%s", arg)
where arg is declared as a fixed-length buffer 'char arg[999];'.
Also the patch is full of arbitrary fixed limits (999, 10), and is inconsistent
between the C code and the Python code about whether there is a depth limit to
the recursion.
Also in src/distcc.c, the code added should be a single function call to a
second function defined in argutil.c.
So the patch is not acceptable in its current state.
Original comment by fer...@google.com
on 16 May 2012 at 6:47
Some other notes on the patch.
Expanding @FILE before dcc_build_somewhere means local compiles will be less
conservative than previously. It is however preferable to perform expansion
before invoking the preprocessor.
Inside libiberty there is exactly the function used by gcc: expandargv. It
does not suffer the issues mentioned in comment #5. Libiberty also includes
other argv helpers similar to the ones used in distcc (duplicate, count, etc.).
Typically it is shipped as a static library (i.e. in Debian binutils-dev).
If there are no objections to introducing this library as a build-time
dependency, then I will rework the patch. Actually, as the library is GPLv2+,
I can include a fallback copy and handle it similar to popt.
Anyway, should be done over the next two days.
Original comment by mand...@gmail.com
on 17 Feb 2013 at 2:58
Note that the previous include_server patch does not implement the same
algorithm as gcc.
Attached is trivial patch to use 'expandargv' in distcc main. This occurs
before any processing is done — including passing command line to server —
so no patching of include_server is required. Works for plain mode, and
confirmed for pump mode by this session:
$ cat args
-I/home/daniel/src/distcc/tmp/argstest
$ pump distcc gcc @args -c -o test.o test.c
…
TRACE: ParseCommand ['gcc', '-I/home/daniel/src/distcc/tmp/argstest', '-c',
'-o', 'test.o', 'test.c']
…
Adds a (typically) build time dependency on libiberty, and distcc binary is
increased by a few kB. There are other functions in libiberty that can
trivially replace much of e.g. distcc/src/argutil.c, but I leave that for a
subsequent patch.
Libiberty is distributed under GPLv2+.
This patch also fixes issue 122.
Original comment by mand...@gmail.com
on 15 Apr 2013 at 1:20
Attachments:
Issue 122 has been merged into this issue.
Original comment by fergus.h...@gmail.com
on 15 Apr 2013 at 9:42
Thanks for the patch!
Looks pretty good, but can you please also update the INSTALL file to document
the new dependency?
Also, I am wondering if it would be better to code this so that if libiberty
isn't found, then we issue a configure-time warning and then fall back to the
previous behaviour of not expanding response files. What do you think?
Original comment by fergus.h...@gmail.com
on 15 Apr 2013 at 10:26
> Also, I am wondering if it would be better to code this so
> that if libiberty isn't found, then we issue a configure-
> time warning and then fall back to the previous behaviour
> of not expanding response files. What do you think?
Some platforms (e.g. in OP) are severly affected. On others failure will be
less common, but apparently random and perhaps difficult to trace back to a
configure time warning.
I would not make this optional, but I suppose the concern is the atypical
distribution of libiberty. Debian packages contain them (binutils-dev), other
systems perhaps not. Aside from the bundled distribution, building and
installing libiberty is a fairly standard procedure that anyone hoping to build
distcc should be capable of.
Including a verbatim copy is suggested by the docs, about 1.8 MB of sources
with decent compression potential.
Original comment by mand...@gmail.com
on 17 Apr 2013 at 5:11
Minimal INSTALL patch for libiberty is required.
Original comment by mand...@gmail.com
on 6 May 2013 at 6:15
Attachments:
The combination of these two patches (expandargv.patch and libiberty-req.patch)
looks good to me. Do you want to go ahead and commit them?
I have added you to the list of users who have permission to commit changes.
See <https://code.google.com/p/distcc/source/checkout> for details on how to
check out the SVN repository.
Original comment by fergus.h...@gmail.com
on 7 May 2013 at 11:37
This issue was closed by revision r774.
Original comment by mand...@gmail.com
on 8 May 2013 at 2:22
Original issue reported on code.google.com by
nilswoet...@gmail.com
on 18 Sep 2011 at 11:13