Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
On 4/19/20, David Kastrup 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.
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Am Sonntag, den 19.04.2020, 23:24 +0200 schrieb David Kastrup: > Jonas Hahnfeld 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 Description: This is a digitally signed message part
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Valentin Villenave writes: > On 4/19/20, David Kastrup 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
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
On 4/19/20, David Kastrup 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.
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Jonas Hahnfeld writes: > Am Sonntag, den 19.04.2020, 22:16 +0200 schrieb David Kastrup: >> pkx1...@posteo.net >> writes: >> >> > Hello, >> > >> > On 19/04/2020 17:53, David Kastrup wrote: >> > > v.villen...@gmail.com >> > > 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".
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Am Sonntag, den 19.04.2020, 22:16 +0200 schrieb David Kastrup: > pkx1...@posteo.net > writes: > > > Hello, > > > > On 19/04/2020 17:53, David Kastrup wrote: > > > v.villen...@gmail.com > > > 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 Description: This is a digitally signed message part
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
pkx1...@posteo.net writes: > Hello, > > On 19/04/2020 17:53, David Kastrup wrote: >> v.villen...@gmail.com 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
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Hello, On 19/04/2020 17:53, David Kastrup wrote: v.villen...@gmail.com 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
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
On 4/19/20, David Kastrup wrote: > v.villen...@gmail.com 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.
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
v.villen...@gmail.com 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
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
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/
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
https://codereview.appspot.com/557640051/diff/565870043/input/regression/font-name-add-files.ly File input/regression/font-name-add-files.ly (right): https://codereview.appspot.com/557640051/diff/565870043/input/regression/font-name-add-files.ly#newcode244 input/regression/font-name-add-files.ly:244: #(rmdir dummyfontdir) So, David ran into a "Directory not empty" error with this regtest, but I haven’t been able to reproduce it. https://sourceforge.net/p/testlilyissues/issues/1204/?page=1#998e/7f0d What could be the cause? V. https://codereview.appspot.com/557640051/
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
On 2020/03/30 18:56:27, thomasmorley651 wrote: > tmpnam is deprecated in guile-3.0.2 (sigh) Nice catch! > Why introducing it, we would need to move away from it later anyway? Well, I *would* have used mkstemp, but I need to create a directory (rather than a file), and there’s no mkdtemp in Guile. Plus, I wanted the code to be supported by both Guile 1.8 and 2. Plus, we’re already using tmpnam somewhere else in the codebase. Plus… > Also, the link you provided speaks about security risks... Yes, so I’ve seen. I certainly *can* imagine how that could be exploited in a critical case. (And by letting users have access to a Turing-complete language within LilyPond files, our exposure to security faults is already quite wide.) Then again, that’s a regtest we’re talking about, not a snippet or a doc example, not something we’re actively encouraging people to do at home. Frankly, if anyone can think of a solution to do the exact same thing with mkstemp (and to remove the temporary files after compilation), I’m all ears. As of right now, I know I can’t :-) V. https://codereview.appspot.com/557640051/
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
On 2020/03/30 18:29:37, Valentin Villenave wrote: > On 2020/03/30 16:06:08, lemzwerg wrote: > > Is it guaranteed that we can create this directory? What about srcdir != > > builddir? > > Yes. It’s a (tmpnam) name, located in /tmp or $TMPDIR or whatever: > https://www.gnu.org/software/guile/manual/html_node/File-System.html#index-tmpnam > That’s the mechanism we already use for intermediate-files. Well, see https://lists.gnu.org/archive/html/guile-devel/2020-03/msg00046.html tmpnam is deprecated in guile-3.0.2 Why introducing it, we would need to move away from it later anyway? Also, the link you provided speaks about security risks... https://codereview.appspot.com/557640051/
Re: Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Reviewers: lemzwerg, Message: On 2020/03/30 16:06:08, lemzwerg wrote: > Is it guaranteed that we can create this directory? What about srcdir != > builddir? Yes. It’s a (tmpnam) name, located in /tmp or $TMPDIR or whatever: https://www.gnu.org/software/guile/manual/html_node/File-System.html#index-tmpnam That’s the mechanism we already use for intermediate-files. The alternative would be to create the font files in the cwd, like what markup-eps.ly does. I decided against it because creating a file is already intrusive, creating a subdir is somehow worse. Now, ideally these should be removed after the PDF is generated. The problem is that (like intermediate-files, precisely) they need to be deleted only after GhostScript’s done with them… and I’m not sure we’re able to overwrite, for example, postprocess-output or output-framework on-the-fly. I don’t think the codebase needs additional hooks only for one regtest to clean after itself, but it *could* be a problem for systems that run the regtests continuously. Thanks for the other nits, I’ve addressed them. V. Description: Issue #1204: Document, and add regtest for, external fonts. Add a few details in NR 1.8.3.1 "Fonts explained", and a regtest with base64-embedded minimal OpenType fonts. (I didn’t want to use musical symbols since the whole point is that these glyphs are not to be confused with whatever’s installed on the OS.) The regtest has been tested with all major versions since 2.12, and current master with and without Guile2. Please review this at https://codereview.appspot.com/557640051/ Affected files (+256, -2 lines): M Documentation/notation/text.itely A input/regression/font-name-add-files.ly
Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Nice idea, thanks. Some nits and questions. https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely File Documentation/notation/text.itely (right): https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely#newcode1552 Documentation/notation/text.itely:1552: (and thus available in LilyPond scores), through the following commands: no comma https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely#newcode1558 Documentation/notation/text.itely:1558: I would remove this empty line in the @example environment. https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly File input/regression/font-name-add-files.ly (right): https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly#newcode14 input/regression/font-name-add-files.ly:14: dummyfontfile = #(string-append (tmpnam) "-dummyfont.otf") s/ / / https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly#newcode210 input/regression/font-name-add-files.ly:210: #(mkdir dummyfontdir) Is it guaranteed that we can create this directory? What about srcdir != builddir? https://codereview.appspot.com/557640051/
