Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

v.villenave
On 2020/04/11 09:44:26, Valentin Villenave wrote:
> What could be the cause?

So, pushed as
https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9

V.

https://codereview.appspot.com/557640051/

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

David Kastrup
[hidden email] writes:

> On 2020/04/11 09:44:26, Valentin Villenave wrote:
>> What could be the cause?
>
> So, pushed as
> https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9
>
> V.
>
> https://codereview.appspot.com/557640051/

Breaks my patchy again.  Whose patchy was comfortable moving it from
staging to master?

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

v.villenave
On 4/19/20, David Kastrup <[hidden email]> wrote:
> [hidden email] writes:
> Breaks my patchy again.  Whose patchy was comfortable moving it from
> staging to master?

Well, at any rate James’ patchy what fine with it. (As it went through
the review/countdown process not just once but twice.)

The regtest actually produces an additional PDF file that should allow
you to check manually that all temporary files have been removed.

Evidently in your case something else (but what?) gets created inside
the temporary directory, which it then can’t remove because it doesn’t
know about it. (I could issue a system call for `rm -rf`, but that
would miss the point.

I already caught one unintended side-effect which was that during make
doc, 'font-export-dir gets enabled and thus my temporary font file
gets referenced which broke make check. There may very well be another
side effect specific to either your system or your build process, but
I’d need help identifying it as I can’t seem to reproduce it myself
(obviously do feel free to revert in the meantime).

Cheers,
V.

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

James Lowe-9
In reply to this post by David Kastrup
Hello,

On 19/04/2020 17:53, David Kastrup wrote:

> [hidden email] writes:
>
>> On 2020/04/11 09:44:26, Valentin Villenave wrote:
>>> What could be the cause?
>> So, pushed as
>> https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9
>>
>> V.
>>
>> https://codereview.appspot.com/557640051/
> Breaks my patchy again.  Whose patchy was comfortable moving it from
> staging to master?
>
Mine.

James


Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

David Kastrup
[hidden email] writes:

> Hello,
>
> On 19/04/2020 17:53, David Kastrup wrote:
>> [hidden email] writes:
>>
>>> On 2020/04/11 09:44:26, Valentin Villenave wrote:
>>>> What could be the cause?
>>> So, pushed as
>>> https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9
>>>
>>> V.
>>>
>>> https://codereview.appspot.com/557640051/
>> Breaks my patchy again.  Whose patchy was comfortable moving it from
>> staging to master?
>>
> Mine.

Ok, by now it has been determined that the breakage on my system likely
depends on a particular fontconfig version.  However, there are a few
other problems with that regtest that should likely show with Guile-2
(sometimes?).  I'll revert for now so that master builds for everyone
again.  That does not preclude a fixed variant to make it into master at
a later point of time.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

Jonas Hahnfeld
Am Sonntag, den 19.04.2020, 22:16 +0200 schrieb David Kastrup:

> [hidden email]
>  writes:
>
> > Hello,
> >
> > On 19/04/2020 17:53, David Kastrup wrote:
> > > [hidden email]
> > >  writes:
> > >
> > > > On 2020/04/11 09:44:26, Valentin Villenave wrote:
> > > > > What could be the cause?
> > > >
> > > > So, pushed as
> > > > https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9
> > > >
> > > >
> > > > V.
> > > >
> > > > https://codereview.appspot.com/557640051/
> > > >
> > >
> > > Breaks my patchy again.  Whose patchy was comfortable moving it from
> > > staging to master?
> > >
> >
> > Mine.
>
> Ok, by now it has been determined that the breakage on my system likely
> depends on a particular fontconfig version.  However, there are a few
> other problems with that regtest that should likely show with Guile-2
> (sometimes?).  I'll revert for now so that master builds for everyone
> again.  That does not preclude a fixed variant to make it into master at
> a later point of time.
To avoid this from happening again, can we please have the rule that
whoever reverts a commit or backs it out of staging is to provide
guidance on what's actually wrong? My feeling is that we could have had
this analysis last week already.

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

David Kastrup
Jonas Hahnfeld <[hidden email]> writes:

> Am Sonntag, den 19.04.2020, 22:16 +0200 schrieb David Kastrup:
>> [hidden email]
>>  writes:
>>
>> > Hello,
>> >
>> > On 19/04/2020 17:53, David Kastrup wrote:
>> > > [hidden email]
>> > >  writes:
>> > >
>> > > > On 2020/04/11 09:44:26, Valentin Villenave wrote:
>> > > > > What could be the cause?
>> > > >
>> > > > So, pushed as
>> > > > https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=0cfef7069e86f85ad392f5c4bc63f6c4801aa2c9
>> > > >
>> > > >
>> > > > V.
>> > > >
>> > > > https://codereview.appspot.com/557640051/
>> > > >
>> > >
>> > > Breaks my patchy again.  Whose patchy was comfortable moving it from
>> > > staging to master?
>> > >
>> >
>> > Mine.
>>
>> Ok, by now it has been determined that the breakage on my system likely
>> depends on a particular fontconfig version.  However, there are a few
>> other problems with that regtest that should likely show with Guile-2
>> (sometimes?).  I'll revert for now so that master builds for everyone
>> again.  That does not preclude a fixed variant to make it into master at
>> a later point of time.
>
> To avoid this from happening again, can we please have the rule that
> whoever reverts a commit or backs it out of staging is to provide
> guidance on what's actually wrong? My feeling is that we could have had
> this analysis last week already.

At that day I was having one patchy run after the other and I did go
through the log files to indicate the failed file and the error message.
I also compiled and ran this by hand and gave the error messages clearly
indicating a non-empty directory.  At some point of time, I dropped the
shoe.  It did not help my focus at that time that Han-Wen was fighting a
contribution of mine nail and tooth.

A "rule" would not have helped since I did not end up dropping the shoe
out of malice.  I went back several times to the problem, just not often
enough to get a final diagnosis.  At some point of time I had to sleep.

Once we determine that I am endangering the project with my wanton
recklessness, we might try reining me in with rules.  And since
basically no-one else bothers reverting commits or backing them out of
staging, such rules would pretty much apply only to me.

It's not like we have an abundance of people to commandeer here.

--
David Kastrup
My replies have a tendency to cause friction.  To help mitigating
damage, feel free to forward problematic posts to me adding a subject
like "timeout 1d" (for a suggested timeout of 1 day) or "offensive".

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

v.villenave
On 4/19/20, David Kastrup <[hidden email]> wrote:
> At that day I was having one patchy run after the other and I did go
> through the log files to indicate the failed file and the error message.

Yep, and I asked for additional info both on the tracker and on
Rietveld; the only reply I did get was from James who confirmed that
the patch was actually running fine on his system, but we agreed to
put it through the reviewing process again. It went through
`reviewing’, `countdown’ and `push’ stages (and languished in the
latter for one more week), at which point I thought I could give it
another go (and I didn’t want to nag you about that any longer, as I
quite noticed that you were focusing on more pressing issues). But at
least this finally allowed us to track down the exact problem, and I
will indeed submit a workaround soon.

> At some point of time, I dropped the
> shoe.  It did not help my focus at that time that Han-Wen was fighting a
> contribution of mine nail and tooth.

Indeed (and your work is still in limbo in that regard, which I hope
gets resolved soon btw, as all this work on the C++ consistency is
much more fundamental and important than some hackish regtest anyway).

I have no objection to this getting reverted (yet again), as long as
I’m now able to understand exactly what went wrong and why, which you
turned out to be the only one who could investigate! So this is
certainly not a case of shoe-dropping as far as I’m concerned.

Cheers,
  - V.

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

David Kastrup
Valentin Villenave <[hidden email]> writes:

> On 4/19/20, David Kastrup <[hidden email]> wrote:
>> At that day I was having one patchy run after the other and I did go
>> through the log files to indicate the failed file and the error message.
>
> Yep, and I asked for additional info both on the tracker and on
> Rietveld; the only reply I did get was from James who confirmed that
> the patch was actually running fine on his system, but we agreed to
> put it through the reviewing process again. It went through
> `reviewing’, `countdown’ and `push’ stages (and languished in the
> latter for one more week), at which point I thought I could give it
> another go (and I didn’t want to nag you about that any longer, as I
> quite noticed that you were focusing on more pressing issues). But at
> least this finally allowed us to track down the exact problem, and I
> will indeed submit a workaround soon.
>
>> At some point of time, I dropped the
>> shoe.  It did not help my focus at that time that Han-Wen was fighting a
>> contribution of mine nail and tooth.
>
> Indeed (and your work is still in limbo in that regard, which I hope
> gets resolved soon btw, as all this work on the C++ consistency is
> much more fundamental and important than some hackish regtest anyway).
>
> I have no objection to this getting reverted (yet again), as long as
> I’m now able to understand exactly what went wrong and why, which you
> turned out to be the only one who could investigate! So this is
> certainly not a case of shoe-dropping as far as I’m concerned.

Ball-dropping.  I really need to get my idioms right.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

Jonas Hahnfeld
In reply to this post by David Kastrup
Am Sonntag, den 19.04.2020, 23:24 +0200 schrieb David Kastrup:

> Jonas Hahnfeld <[hidden email]> writes:
> > To avoid this from happening again, can we please have the rule that
> > whoever reverts a commit or backs it out of staging is to provide
> > guidance on what's actually wrong? My feeling is that we could have had
> > this analysis last week already.
>
> At that day I was having one patchy run after the other and I did go
> through the log files to indicate the failed file and the error message.
> I also compiled and ran this by hand and gave the error messages clearly
> indicating a non-empty directory.  At some point of time, I dropped the
> shoe.  It did not help my focus at that time that Han-Wen was fighting a
> contribution of mine nail and tooth.
>
> A "rule" would not have helped since I did not end up dropping the shoe
> out of malice.  I went back several times to the problem, just not often
> enough to get a final diagnosis.  At some point of time I had to sleep.
>
> Once we determine that I am endangering the project with my wanton
> recklessness, we might try reining me in with rules.  And since
> basically no-one else bothers reverting commits or backing them out of
> staging, such rules would pretty much apply only to me.
>
> It's not like we have an abundance of people to commandeer here.
That's certainly not what I wanted to imply.
My consideration is that this patch went through the countdown twice
(and then some). It passed all tests before pushing, yet got reverted.
The failure was apparently hard to reproduce (which makes the case
tricky), so maybe this is just beyond what usually happens with a
patch?
Please take my statements as thoughts about how to improve experience
for contributors. I think there are better ways to spend your time than
guessing which part of your patch broke on a particular system.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villenave@gmail.com)

v.villenave
In reply to this post by David Kastrup
On 4/19/20, David Kastrup <[hidden email]> wrote:
> However, there are a few
> other problems with that regtest that should likely show with Guile-2
> (sometimes?).

Could you elaborate as to what I need to watch out for? I’ve tested it
successfully with Guile 2.2 here (but history has teached us that
unforeseen stuff can happen on various setups…).

Meanwhile, I’ve posted a fix for rmdir here:
https://codereview.appspot.com/573730044
(git-cl created a new page as I had deleted the temporary git branch
I’d previously been working with).

Cheers,
  - V.