Re: CVS commit: pkgsrc/lang/racket-textual

[Aleksej Saushev <asau%inbox.ru@localhost>]

John Marino <netbsd%marino.st@localhost> writes:

> On 9/24/2012 20:20, Aleksej Saushev wrote:
>> John Marino<netbsd%marino.st@localhost>  writes:
>>
>>> On 9/24/2012 13:18, Aleksej Saushev wrote:
>>>> Module Name:       pkgsrc
>>>> Committed By:      asau
>>>> Date:              Mon Sep 24 11:18:38 UTC 2012
>>>>
>>>> Modified Files:
>>>>    pkgsrc/lang/racket-textual: Makefile
>>>>
>>>> Log Message:
>>>> Revert changes that make no sense.
>>>>
>>>>
>>>> To generate a diff of this commit:
>>>> cvs rdiff -u -r1.11 -r1.12 pkgsrc/lang/racket-textual/Makefile
>>>>
>>>> Please note that diffs are not public domain; they are subject to the
>>>> copyright notices on the relevant files.
>>>>
>>>
>>>
>>> Translation: Alexsej just changed all the "{}" back to "()" for
>>> no benefit, and to apparently to intentionally cause pkglint to
>>> generate warnings for the reason I can only speculate that he
>>> thinks pkglint should not classify "()" as an error.
>>>
>>> This revert had no justification and it makes the pkgsrc
>>> Makefile worse (same story with lang/racket).   This is
>>> irresponsible.  For a lesser extent, OBATA Akio pointed me to
>>> commit guidelines that said one should avoid reverting commits
>>> of other developers and that was violated as well.
>>>
>>> Apparently there's no point on using pkglint, at least on
>>> packages that asau has interest in.
>>
>> This reversion has benefit to me as maintainer at the very least.
>>
>> pkglint is broken, and I have raised this problem previously.
>> First and the biggest problem with pkglint is that it insist on
>> undocumented rules with unclear reasoning (sometimes harmful ones)
>> which brings it into category of "probably good, if used with care"
>> at most rather than a tool to remove mistakes.
>> Thus instead of following all pkglint suggestions blindly, please,
>> think about the reasoning behind all what happens there.
>>
>> You have not consulted with me about changing these packages, even though
>> I maintain them and I use them. In addition, there're more packages that
>> follow these. All that you have made is introduction of pointless and
>> harmful (at least, potentially) changes that cause problems to me as
>> package maintainer.
>>
>> The same applies to using non-tested JPEG library (I have never tested
>> Racket with that another option and have no time to test it for now)
>> and using LOCALBASE (which is dubious change as well, though it might
>> have some sense which is why I didn't revert it).
>>
>> It is not the first time you introduce changes that have very little
>> reason behind them (if any at all), sometimes they have only aesthetic
>> value and sometimes are just harmful ad-hockery. In future, please,
>> provide clear reasoning. Otherwise, I'll have to revert non-functional
>> changes that get in a way rather than serve any purpose in packages
>> I maintain.
>
>
> I know for a fact that using "()" instead of "{}" causes some
> pkglint logic to fail.  All things being equal, "{}" is
> therefore superior to "()" where pkglint is concerned.   Rather
> than have to reprogram pkglint for both "()" and "{}" cases, it
> makes perfect sense to flag one version as "bad".   This doesn't
> even address consistency which is another valid reason to allow
> only one case.

No. It doesn't make it "perfect sense". In fact, there're reasons why
I have converted to using parentheses (it helps diagnosing rather tricky
problems much easier). You have reverted the change I have made
consciously just to appease some tool that cannot even parse makefile
syntax correctly.

> It's one thing to not bother to change them on principal.  It's
> quite another to go out of your way to revert a legitimate
> commit in order to make the Makefile "wrong" in the eyes of
> pkglint.
>
> This commit is nothing more than childish protest against
> pkglint. pkglint is an official pkgsrc tool, and cleaning up
> pkglint warnings and messages is a valid reason in an of itself.
> Obviously if you claim most of these changes are aesthetic, you
> are clearly admitting they make no difference either way.  And
> if you have such a mad-on about pkglint, fix it.

No, the only childish thing here is blindly following pkglint
recommendations reasons for which you don't understand.

If you want detailed analysis what is wrong with your commit, here it is.

There're three changes that are made under umbrella of pkglint "cleanup":
1. Conversion from human parentheses to curly braces, which at least is
purely cosmetic issue that has absolutely no value.
2. Replacing usage of LOCALBASE with PREFIX.
3. Linking against _some_ JPEG library instead of particular one.

All three have the following problems:
1. Conversion from human parentheses to curly braces is purely cosmetic
change that has absolutely no (positive) value otherwise.
1.1. Using curly braces complicates important (at least to me) case
when diagnosing problems of interaction with shell. It is much easier to
diagnose certain kind of problems with parentheses but not curly braces.
1.2. The change effectively reverts my conscious choice.
1.3. All the change does is in the name of pkglint that fails to parse
makefile syntax (while pretending to do a sort of semantic analysis, ha-ha).
2. Using PREFIX instead of LOCALBASE is pointless.
2.1. Currently LOCALBASE and PREFIX are equivalent in overwhelming
majority of cases.
2.2. While there're use cases when the difference makes sense,
I seriously doubt that you thought of them or that you tried them.
2.3. In fact, the change from LOCALBASE to PREFIX goes against
established practice for interpreters.
See lang/python/application.mk and lang/python/pyversion.mk for instance
(and this is what it was modelled on).
3. Linking against _some_ JPEG library instead of tried one is even more
problematic than above.
3.1. I doubt that you tested it, but you have effectively allowed using it.
You haven't asked a question why this is done exactly that way, while
it is possible that subtle differences in implementations may cause
problems due to the way the library is used (through some FFI).
The latter problems are really hard to debug (I have FFI debugging
experience in other compilers).
3.2. In addition to previous, you have left almost no time to test it.

In addition to all above, your previous commit is hackish and may cause
problems already (you haven't consulted man page on bzero you used,
and I doubt that you checked whether this function does exist on all
platforms of interest).

Claiming that "cleaning up pkglint warnings and messages is a valid
reason in an of itself" is a large overstatement. Having looked at
pkglint internals, I can assert exactly the opposite:
(blindly) following pkglint is invalid reason for any changes.
First (and foremost), pkglint is written in a way that it doesn't follow
make syntax. This alone downgrades pkglint's recommendations from being
sort of "absolute" to probabilistic "there may be problems".
Second, there's no human readable specification to what pkglint recommends
(and no demonstration that it does implement specification correctly).
Third, the code itself raises doubts whether it does reflect current practice.
Code's quality is a bit dubious either, in my opinion.


-- 
HE CE3OH...