Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added code to handle more oddities in file naming. #328

Merged
merged 12 commits into from
Apr 5, 2019

Conversation

henryso
Copy link
Contributor

@henryso henryso commented Aug 12, 2018

Please note that this changes the VIEW / DIFF settings in the gregorio-test.rc file. See the example file for the required format. Basically, what used to be a string should now be an array. For example, what was:

VIEW_TEXT="less {file}"

should now be

VIEW_TEXT=( less {file} )

This is to support weird filenames for gregorio-project/gregorio#1426.

This also pulls in some bug fixes from the develop branch, but these are acceptable for the ctan branch as well.

@rpspringuel
Copy link
Contributor

This fix isn't working for me because of the output directory. The testing program creates fix'1416.tex.out as the destination for the output of the LuaLaTeX compile. However, GregorioTeX, using the renaming rules I've given it outputs the following warnings:

Module gregoriotex Warning: fix'1416.tex.out/fix'1416.gtmp was renamed to fix-14
16.tex.out/fix-1416.gtmp on input line 135
Module gregoriotex Warning: fix'1416.tex.out/fix'1416.test.gsnippet was renamed 
to fix-1416.tex.out/fix-1416.test.gsnippet on input line 135
Module gregoriotex Warning: fix'1416.tex.out/fix'1416.gsnippet was renamed to fi
x-1416.tex.out/fix-1416.gsnippet on input line 135
Module gregoriotex Warning: fix'1416.tex.out/fix'1416.gsniplog was renamed to fi
x-1416.tex.out/fix-1416.gsniplog on input line 135

As you can see in those warnings, the output directory is part of the protected filename and the renaming rules "fix" it. Either the testing program has to create a "safe" output directory or the renaming rules have to be made aware of the possibility of an output directory and not play with its name when making the file names "safe". I'm not sure at this point which is the better option and will have to study the problem when I'm not going to bed.

@henryso
Copy link
Contributor Author

henryso commented Aug 13, 2018

It's possible, I suppose, to alter the directory created for the output, but doesn't that indicate a potential issue when a user uses a directory with problematic characters in it?

@henryso
Copy link
Contributor Author

henryso commented Aug 13, 2018

I looked a bit at the lua code, and it makes me think that perhaps it would be better to quote/escape things in the command than to fiddle with the arguments. The difficulty with this is OS-compatibility, unless there is an explicit form of execution with explicit arguments.

@rpspringuel
Copy link
Contributor

but doesn't that indicate a potential issue when a user uses a directory with problematic characters in it?

That's what has me vacillating on whether the solution should be changing the testing protocol or the program behavior. I need to do some testing as to what happens currently and with my proposed changes when the directory name contains a protected character.

@henryso
Copy link
Contributor Author

henryso commented Aug 15, 2018

Have you tried quoting the arguments? If it works, I think it would cover most of the cases. Should be compatible with both Posix and Windows. To wit:

l.150/156. local cmd = string.format([["%s" -o "%%s" "%s"]], real_gregorio_exe, test_snippet_filename)
l.935. local cmd = string.format([["%s" -W %s-o "%%s" -l "%s" "%s"]], gregorio_exe(), deprecated, snippet_logname, snippet_filename)

@rpspringuel
Copy link
Contributor

Trying that (after undoing the other changes I had made) works for the new test, but now I'm getting a bunch of the following errors:

gabc-output/bugs/fix-347.gabc : FAIL - Expectation missing for gabc-output/bugs/fix-347.gabc.out/fix-347/fix-347.tex

As you can see there's an extra level of directory introduced in the the path down which the testing program searches for the expectation. I'm not sure what might be causing that as it's only happening on 16 tests. I'm going to have to look more closely at those tests to see what they might have in common (and which is different from all the others) in order to figure out why they are having this issue.

@henryso
Copy link
Contributor Author

henryso commented Aug 18, 2018

That seems odd indeed. Let me see if I can figure out what's going on. It smells like a bug in the test harness implementation rather that in gregorio.

@henryso
Copy link
Contributor Author

henryso commented Aug 18, 2018

I ran the filenames branch of gregorio-test against the ctan branch of gregorio with only the modifications to the three lines above. I don't get the symptoms you get. The most likely possibilities that come to mind are (1) something I didn't commit (it doesn't look like it), (2) some incompatibility with the version of bash I'm using, or (3) an OS incompatibility.

my bash --version shows:

GNU bash, version 4.4.12(1)-release (x86_64-pc-linux-gnu)

What version are you using?

@rpspringuel
Copy link
Contributor

GNU bash, version 4.4.23(1)-release (x86_64-apple-darwin17.5.0)

Looks like I have a more recent version, but only at a bug-fix level.

@henryso
Copy link
Contributor Author

henryso commented Aug 19, 2018

I'm unfortunately at a loss as to the problem. I think I have to depend on your ability to debug this.

@henryso
Copy link
Contributor Author

henryso commented Aug 19, 2018

Also, if there's any other information you can give me that might help me focus my efforts, that would be helpful. For example, what is the command you are using to invoke the tests?

When an expectation image cannot be found the image comparison is skipped and the test fails.  Previously, however, this was reported as a "Results differ" failure, implying that the comparison was run.  This change makes it so that the reported error message indicates the real cause of the test failure (missing expectation image).
The same directory gets referenced several times in a row (and will be referenced even more often in the next commit) so I store it in a variable.
When the `convert` command to populate an image-cache directory fails, we remove the empty directory.  We do this because subsequent runs of the script will use the existence (and timestamp) on the directory to determine if the image-cache can be used for a particular test and an empty directory would falsely indicate that the image-cache was available.
This test is named as testing a bug, but wasn't in the bugs folder.  Thus I move it.
@rpspringuel rpspringuel mentioned this pull request Mar 30, 2019
rpspringuel added a commit to rpspringuel/gregorio that referenced this pull request Mar 30, 2019
This fix for problematic filenames seems to work and is simpler than what I was working on in gregorio-project#1426.
It is compatible with gregorio-project/gregorio-test#328 after the changes I proposed on henryso/gregorio-test#1 are pulled in.
The corresponding PR which adds the test for this problem is gregorio-project/gregorio-test#331
@henryso henryso merged commit 4861c28 into gregorio-project:ctan Apr 5, 2019
@henryso henryso deleted the filenames branch April 5, 2019 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants