The Trunk: Traits-ar.278.mcz

Andreas Raab uploaded a new version of Traits to project The Trunk:
http://source.squeak.org/trunk/Traits-ar.278.mcz

==================== Summary ====================

Name: Traits-ar.278
Author: ar
Time: 9 January 2010, 5:57:39.959 pm
UUID: 55a356f8-f654-df4d-a991-dd6ccc464fbe
Ancestors: Traits-nice.277

Adds a class comment to TraitOrganizer (really this is to test the updated server image).

=============== Diff against Traits-nice.277 ===============

Item was changed:
  ClassOrganizer subclass: #TraitOrganizer

  instanceVariableNames: 'traitComposition'

  classVariableNames: ''

  poolDictionaries: ''

  category: 'Traits-Kernel'!

+

+ !TraitOrganizer commentStamp: 'ar 1/9/2010 17:56' prior: 0!

+ A class organizer containing state for traits.!

On Sun, 10 Jan 2010, [hidden email] wrote:

I tried to log in, but something went wrong:
Request handling aborted; reload to retry


Levente

Levente Uzonyi wrote:
> I tried to log in, but something went wrong:
> Request handling aborted; reload to retry

I was trying to understand what goes wrong but how do you debug code
that relies intrinsically on continuations? I'm really at a loss here;
this works when I run it on my windows box but fails on
source.squeak.org. Any ideas what to try next? If not, I'll have to
revert to the old version.

Cheers,
   -Andreas

On Sun, 10 Jan 2010, Andreas Raab wrote:

> Levente Uzonyi wrote:
>> I tried to log in, but something went wrong:
>> Request handling aborted; reload to retry
>
> I was trying to understand what goes wrong but how do you debug code that
> relies intrinsically on continuations? I'm really at a loss here; this works
> when I run it on my windows box but fails on source.squeak.org. Any ideas
> what to try next? If not, I'll have to revert to the old version.

What's the curret ErrorHandler class?
WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a
mail to the superuser) can help tracking the issue.


Levente

>
> Cheers,
>  -Andreas
>
>
>

Levente Uzonyi wrote:
> What's the curret ErrorHandler class?
> WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a
> mail to the superuser) can help tracking the issue.

These classes aren't even present in our current version.
See http://source.squeak.org/ss.html

Cheers,
   - Andreas

On Sun, 10 Jan 2010, Andreas Raab wrote:

> Levente Uzonyi wrote:
>> What's the curret ErrorHandler class?
>> WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a mail
>> to the superuser) can help tracking the issue.
>
> These classes aren't even present in our current version.
> See http://source.squeak.org/ss.html

I see, it's too old. There's WAEmailErrorPage.


Levente

>
> Cheers,
>  - Andreas
>
>

Levente Uzonyi wrote:
Should be okay now. It looks like there was some problem with a seaside
extension method that was somehow lost. The good news is we're now
running current code (update 8824) and no longer the ancient 3.7 based
version.

Cheers,
   - Andreas

On 10.01.2010, at 04:46, Andreas Raab wrote:
>
> The good news is we're now running current code (update 8824) and no longer the ancient 3.7 based version.
>
> Cheers,
>  - Andreas

Rock!

Let's watch if that fixes the next Multilingual package upload ...

- Bert -

Bert Freudenberg wrote:
> Rock!
>
> Let's watch if that fixes the next Multilingual package upload ...

Works all right so far, but I think we still got two problems:

1) Does anyone have an idea where the double line ends come from when
mailing out the diffs? It's really annoying. Any pointers to where to
start looking at would be greatly welcome.

2) Does anyone feel as if we're getting a higher number of connection
failures than usual? It could be an issue on my end but I somehow doubt
that. I seem to getting connection failures about 20% of the time when
trying to update. It's quite noticable.

Cheers,
   - Andreas

On Sun, 10 Jan 2010, Bert Freudenberg wrote:

It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.


Levente

>
> - Bert -
>
>
>

On Sun, 10 Jan 2010, Andreas Raab wrote:

I guess it's because TextDiffBuilder now keeps the original line endings,
so lines with different line endings will not match. If someone fixes line
endings (removing lf's) the diffs can reflect that.

>
> 2) Does anyone feel as if we're getting a higher number of connection
> failures than usual? It could be an issue on my end but I somehow doubt that.
> I seem to getting connection failures about 20% of the time when trying to
> update. It's quite noticable.
>

I didn't get any, so far.


Levente

> Cheers,
>  - Andreas
>
>

Levente Uzonyi wrote:
> It works, but there's a new (minor) issue: the diff contains double line
> endings. And the old green image was better than the current blue on the
> web interface, because other style elements are for the green image.

Both should be fixed now.

Cheers,
   - Andreas

On Sun, 10 Jan 2010, Andreas Raab wrote:

> Levente Uzonyi wrote:
>> It works, but there's a new (minor) issue: the diff contains double line
>> endings. And the old green image was better than the current blue on the
>> web interface, because other style elements are for the green image.
>
> Both should be fixed now.

There are (at least) three ways to fix the extra empty lines issue:
1. restore the original behavior in TextDiffBuilder >> #split: by using
   endWithoutSeparators instead of end.
2. update TextDiffBuilder >> #printTextPatchSequence:on: to add cr if and
   only if the line doesn't end with cr or crlf.
3. create a subclass of TextDiffBuilder that overrides #split: and throws
   away line endings. and use that from MCDiffyTextWriter. The extension
   methods could be moved there too.


Levente

>
> Cheers,
>  - Andreas
>
>

On Sun, 10 Jan 2010, Levente Uzonyi wrote:

I finally fixed it in a fourth way by making #buildPatchSequence
more backwards compatible by ignoring crs (System-ul.230). A fifth
solution would be to change #buildTextPatch to use
#patchSequenceDoIfMatch:ifInsert:ifRemove: directly, like:

TextDiffBuilder >> #buildTextPatch

    ^String streamContents: [ :stream |
       self
          patchSequenceDoIfMatch: [ :string |
             stream space: 2.
             self print: string withAttributes: nil on: stream ]
          ifInsert: [ :string |
             stream nextPutAll: '+ '.
             self print: string withAttributes: nil on: stream ]
          ifRemove: [ :string |
             stream nextPutAll: '- '.
             self print: string withAttributes: nil on: stream ] ]

This would also allow us to remove #stringForAttributes: and
#printTextPatchSequence:on:.


Levente

Levente Uzonyi wrote:
I think I'll leave that decision to you, you seem to have a good handle
on this part of the system. FWIW, I had implemented option #2 for our
SqueakSource installation realizing that it would be robust even if we
decided to leave out the CRs from the diff.

Cheers,
   - Andreas

On Sun, 10 Jan 2010, Andreas Raab wrote:

With the latest TextDiffBuilder changes everything should work fine with
all versions of SqueakSource. #buildTextPatch is SqueakSource's extension
method, changing that would break backwards compatibility.


Levente

>
> Cheers,
>  - Andreas
>
>

Levente Uzonyi wrote:
> On Sun, 10 Jan 2010, Andreas Raab wrote:
>> I think I'll leave that decision to you, you seem to have a good
>> handle on this part of the system. FWIW, I had implemented option #2
>> for our SqueakSource installation realizing that it would be robust
>> even if we decided to leave out the CRs from the diff.
>
> With the latest TextDiffBuilder changes everything should work fine with
> all versions of SqueakSource. #buildTextPatch is SqueakSource's
> extension method, changing that would break backwards compatibility.

Yup, agreed. It was a quick localized fix for our installation. I didn't
feel like messing around with TextDiffBuilder itself - making the change
in the one extension method felt safer for our purposes. Thanks for
fixing the issue at the root!

Cheers,
   - Andreas

On Sun, 10 Jan 2010, Andreas Raab wrote:

I did a bit of stress testing, by uploading a few packages one after
another. There was no problem during the uploads, but when all uploads
were finished it took 20-30 seconds for the server to respond for the next
few 1-2 minutes. I guess it's related to the gc settings, so tweaking them
may help. For a reference, here are the "seaside defaults":

SmalltalkImage current
    vmParameterAt: 5 put: 100000;
    vmParameterAt: 6 put: 35000;
    vmParameterAt: 24 put: 16 * 1024 * 1024;
    vmParameterAt: 25 put: 8 * 1024 * 1024.
Smalltalk
    setGCBiasToGrowGCLimit: 16 * 1024 * 1024;
    setGCBiasToGrow: 1

The uploaded changes were produced with the code critics feature of
OmniBrowser. I think we should use it regularly, because it can point out
issues which could be hard to find otherwise and there are plenty of
issues to be fixed. They are bit broken at the moment, so running them on
the whole image needs a few fixes.
Some issues are easy to fix, others require deep understanding of the
system.

Some categories which should be fixed (number of possible issues):
Messages sent but not implemented (571)
Sends unknown message to global (36)
Subclass responsibility not defined (82)
Instance variable capitalization (4)
Temporary variable capitalization (5)
Inconsistent method classification (324)
Non-blocks in special messages (24)
Unnecessary assignment or return in block (45)
Uses "(a and: [b]) and: [c]" instead of "a and: [b and: [c]]" (78)
Uses (to:)do: instead of to:do: (23)
Variable is only assigned a single literal value (44)
Instance variable overridden by temporary variable (21)
Possible missing "; yourself" (490)
Returns a boolean and non boolean (42)
Subclass of collection that has instance variable but doesn't define copyEmpty (1)
Menus missing translations (75)
Method source contains linefeeds (123)
Assignment has no effect (22)
Check for same statements at end of ifTrue:ifFalse: blocks (43)
Instance variables not read AND written (209)
Method just sends super message (17)
Variable referenced in only one method and always assigned first (69)
Variables not referenced (266)

I hope we can decrease these numbers soon.


Levente

> Cheers,
>  - Andreas
>
>

Hi Levente,
what about completely ignoring line endings in diffs ?

split: aString
        "I return an Array of strings which are the lines extracted from
aString. All lines contain the line separator characters"

        ^Array streamContents: [ :stream |
                aString lineIndicesDo: [ :start :endWithoutSeparators :end |
                        stream nextPut: (aString copyFrom: start to: endWithoutSeparators) ] ]

or simply


split: aString
        "I return an Array of strings which are the lines extracted from
aString. All lines contain the line separator characters"

        ^Array streamContents: [ :stream |
                aString linesDo: [ :aLineWithoutEnding |
                        stream nextPut: aLineWithoutEnding ] ]

Nicolas

2010/1/11 Andreas Raab <[hidden email]>:

On Mon, 11 Jan 2010, Nicolas Cellier wrote:

> Hi Levente,
> what about completely ignoring line endings in diffs ?
>

I intentionally added this feature. Do you think it's wrong?


Levente