[Scons-dev] Scons 2.3.2 regression, D tool...

[anatoly techtonik]

On Fri, Aug 8, 2014 at 8:07 PM, Dirk Bächle <tshortik at gmx.de> wrote:
> Hi there,
>
> we have another user reporting trouble with SCons 2.3.2, in connection to
> the D tool...so it's time for us to act. I checked the sources, and the
> user's analysis appears to be partly correct: even when no D tool is present
> in the current system, the "dmd" tool gets loaded (meaning
> SCons.Tool.dmd.generate is executed).

There is an explicit check on "dmd" binary. What is "dmd" binary then?
https://bitbucket.org/scons/scons/commits/b757fe34f9fe#Lsrc/engine/SCons/Tool/dmd.pyF233T135

> In general, the D tools use their own set of variables starting with "D*"
> (or "_D*") so they shouldn't create much of a problem. But the D tools also
> set "STATIC_AND_SHARED_OBJECTS_ARE_THE_SAME" to 0, independent of any
> setting that a tool like msvs.py made before. :(

Why D uses global keyword that is reserved for C stuff?

> I'm open for a discussion and would like to hear other ideas. My way to fix
> this would be:
>
> - D tools shouldn't be loaded by default, but get an optional tool ( ->
> tools=['default', 'dmd'] is mandatory). It's not enough to say "Let's try to
> detect them, and load them automatically.", as long as the "STATIC_SHARED"
> flag gets set to a fixed value. An option would be to use env.Default() to
> set this flag, but I don't know if D can then cope with a possible setting
> of "1" (Russel?).

+1 on disabling it by default for the performance reasons listed here
https://bitbucket.org/scons/scons/commits/b757fe34f9fe#chg-src/engine/SCons/Tool/__init__.py

> - Docs should get amended, to warn the user about the overwrite of the
> "STATIC_AND_SHARED" flag.
> - Then push out a new patch release as soon as possible.

The problems are caused by this commit:
https://bitbucket.org/scons/scons/commits/b757fe34f9fe
which is huge (which is bad), which is also a source of this bug
http://scons.tigris.org/issues/show_bug.cgi?id=2966
in which Gary accepts that the commit is large, but since he is the one
who merged it, I guess it didn't pass proper review
https://bitbucket.org/scons/scons/commits/40fa954282a3893b809eb4782efe231a95ded10f
and the worse part that was already reverted once
https://bitbucket.org/scons/scons/commits/8764000345e06e326ef68fd0acf9366c1f3eb885
which raises a question about our review process and debt
of competence.

I made a quick review too, and noticed a new default tool, but the fact
that D tool became new default tool that *changes default behavior* is
something that I didn't think might happen. But what it is not reflected
in CHANGES.txt I think is a major ommision.

Now a big thing. CHANGES.txt explicitly says  "Tools for DMD, GDC
and LDC provided and integrated with the C and C++ linking." That should
say something for people who now C/C++ (i.e. not to me). Looking at pull
request,  I don't see tests that cover this integration on C and C++ side.
The tests need to be updated in any case.

About solving regressions with documentation - I am not a C/C++ coder,
but I am not a fan of dealing with things this way.

P.S. I like this cause from PyPy
http://doc.pypy.org/en/latest/how-to-contribute.html?highlight=hacking#don-t-just-hack
think any more or less popular Python project should adopt it.

-- 
anatoly t.