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

Fix some errors related to .glog file #1632

Merged
merged 24 commits into from
Feb 26, 2025

Conversation

davidweichiang
Copy link
Contributor

  • Make gregorio overwrite the .glog file instead of appending to it
  • If there is an error opening the .glog file, write the error to stderr (previously the error could go nowhere)
  • More fixes not implemented yet

Addresses #1628

- If there is an error opening the .glog file, write the error to stderr (previously the error could go nowhere)
…kay because the only error that gregorio prints to stderr (instead of the .glog file) is if the .glog file itself is not writable.

- Eliminate some duplicate code.
@rpspringuel rpspringuel changed the base branch from develop to release-6.1 February 20, 2025 20:39
@rpspringuel
Copy link
Contributor

I've switched this to be against release-6.1 instead of develop. Please make sure that this hasn't broken anything.

- don't search for .gtex file using kpse; instead always use tmp-gre
- don't clean up old .gtex files
- If there is an error opening the .glog file, write the error to stderr (previously the error could go nowhere)
…kay because the only error that gregorio prints to stderr (instead of the .glog file) is if the .glog file itself is not writable.

- Eliminate some duplicate code.
- don't search for .gtex file using kpse; instead always use tmp-gre
- don't clean up old .gtex files
@davidweichiang
Copy link
Contributor Author

It's difficult to test all the possible combinations, but the core cases definitely work against release-6.1. Only one test is really failing, fancyhdr-hooks, which is something new, right?

@rpspringuel
Copy link
Contributor

Yes, fancyhdr-hooks is new. How is it failing?

Sent with GitHawk

@davidweichiang
Copy link
Contributor Author

fancyhdr-hooks:

image

@rpspringuel
Copy link
Contributor

That’s breaking fancyhdr-hooks back to what it was before the fix. Are you testing from a branch which doesn’t have that fix applied?

Sent with GitHawk

…ell and therefore doesn't need quoting. Previously, filenames containing ' were okay, but filenames containing " were not; now, both should be okay.
…ell and therefore doesn't need quoting. Previously, filenames containing ' were okay, but filenames containing " were not; now, both should be okay.
@davidweichiang
Copy link
Contributor Author

It appears to be because my fancyhdr is too old (v4.1, which was in TeXLive 2024) and hooks have only been around since v4.5 (Dec 2024).

- .glog files are also deleted
- Files are deleted just before every run of gregorio (previously it was only when the .gtex file was missing)
- Fixed a possible bug where even on Windows, a pathname was matched against a pattern containing /
@davidweichiang
Copy link
Contributor Author

We decided to remove these files to prevent excessive clutter of gtex files made for old versions of gregorio. Since these gtex files were created by gregorio we felt that gregorio should be allowed to remove them without user input.

OK, this code is restored; actually it now deletes more files than it used to:

  • .glog as well as .gtex
  • in both the old location (same as .gabc) and new location (tmp-gre)
  • whenever gregorio is run (previously it was just when the .gtex file was missing)

@davidweichiang davidweichiang marked this pull request as ready for review February 21, 2025 15:05
@davidweichiang
Copy link
Contributor Author

I just pushed a couple of bug fixes, and a "feature" for replacing .. with dotdot.

@rpspringuel
Copy link
Contributor

Testing against d3e4bd6

File locations specified relative to Main.tex

Compiling from the main directory (the location of the tex file):

Works. Directory structures are created as expected (alongside is under tmp-gre/dotdot).

Compiling from Test_gregorio_dev (parent of main):

Scores cannot be found since locations are specified relative to Main.tex and not the current working directory. Directory structure of tmp-gre is as expected, however.

Compiling from alongside:

Scores in alongside and main are found (as they are in the same location relative to alongside as they are relative to main). Scores in main/subdir and main/subdir are not. Directory structure of tmp-gre is as expected.

Compiling from subdirectories:

Scores in main and subdir are found (coincidence due to directory names). Scores in subdir/subdir and alongside are not. Directory structure of tmp-gre is as expected.

File locations specified with \gresetgregpath{{../}}

Compiling from the main directory (the location of the tex file):

Works. Directory structure of tmp-gre is as expected (alongside is directly under tmp-gre since we don't have .. in the path under this condition).

Compiling from Test_gregorio_dev (parent of main):

Works.

Compiling from alongside:

Works.

Compiling from subdirectories:

No scores found. Directory structure of tmp-gre is as expected. Changing to \gresetgregpath{{../../}} results in file compiling just fine.

My VM is still installing TeX Live (my main system idled the VM earlier, so the install got paused over lunch, grr...). I'm probably a couple of hours from being able to test on Windows, so that likely means testing tomorrow.

@davidweichiang
Copy link
Contributor Author

That sounds better…some of these conditions are not expected to work, right? Are there remaining bugs?

@rpspringuel
Copy link
Contributor

The only ones I would expect to work are compiling from the directory of the file itself and compilations where \gresetgregpath has been used to compensate for an out of directory invocation. Both of those currently work (as well as some others).

We've thought about doing something different in the past (here's a post from Elie from 2015 where he appears to have been working on this), but since this would be different from how \input and \include work, I'm not feeling any pressure to implement it.

Windows testing results:

File locations specified relative to Main.tex

Compiling from the main directory (the location of the tex file):

Score in main is found, others are not. File name normalization doesn't appear to be active. I'm getting errors involving path names with both \ and / present. Probably need to apply lfs.normalize to filenames before opening them and calling executable (see #1622 for example). We also seem to be missing a path separator when making directories as tmp-gresubdir is created rather than tmp-gre/subdir.

Testing from other directories not done. I'm out of time and the above needs to be fixed anyway, so I might as well do those tests after that fix is applied.

@davidweichiang
Copy link
Contributor Author

Quick question about backslashes. The function lfs.normalize changes all \ to /, and the comment says that on Windows, / is actually okay internally, but users might type \ so they need to be changed to /. That appears to be the opposite of the strategy taken in other parts of the code, where a variable sep is set to \ on Windows (or is supposed to be -- I see your comment above). Are you able to confirm that the comment on `lfs.normalize is correct, and if so, should the same strategy be applied everywhere?

@rpspringuel
Copy link
Contributor

My earlier exchange with Akira indicated that the directory separator should be / in Lua and TeX files, even on Windows (that's what his path in #1622 was designed to do). I agree our code seems to have gone for the exact opposite strategy at some point, I'm not sure why. Perhaps something changed in LuaTeX to make the internal handling of filenames more consistent? In any event, I think we should drop the whole \ thing in our own code and make use of lfs.normalize to clean up filenames provided by users as necessary. I think that will fix our problems with finding scores on Windows and making the directories under tmp-gre.

@davidweichiang
Copy link
Contributor Author

Does kpse.find_file return paths with / or \?

@rpspringuel
Copy link
Contributor

I don't know. I'd have to test it (and I'm headed to class right now). If you can provide a simple test file (one that say, uses \directlua to call that function, I'll test it during my lunch break (otherwise it'll have to wait until the school day is over).

…herever a pathname component could be introduced

- only replace `..` with `dotdot` when it is an entire pathname component
@davidweichiang
Copy link
Contributor Author

I just went ahead and applied lfs.normalize to the output of kpse.find_file, and a couple of other places where I wasn't certain.

@rpspringuel
Copy link
Contributor

Windows testing against the new head:

Compiling from main

Works. tmp-gre is created and structured as expected (with alongside below (dotdot).

Compiling from Test_gregorio_dev

No scores found. tmp-gre is not created.

Compiling from alongside

Scores in main and alongside are found. Scores in subdir and subdir/subdir are not. tmp-gre is created with dotdot/alongside inside, but no subdir.

Compiling from subdir

Scores in main and subdir are found (coincidence because subdir and subdir/subdir exist and contain the same score). Scores in subdir/subdir and alongside are not. tmp-gre is created with subdir inside, but not dotdot.

Changing paths and setting \gresetgregpath{{../}}

Compiling from main

No scores found. tmp-gre not created. Surprised this one doesn't work. Looking in the log I see the following error:

gregoriotex.lua:1054: attempt to index a nil value (local 'path')
stack traceback:
        ...xlive/texmf-local/tex/luatex/gregoriotex/gregoriotex.lua:1054: in function 'lfs.normalize'
        ...xlive/texmf-local/tex/luatex/gregoriotex/gregoriotex.lua:1227: in field 'include_score'

Are we ending up with an empty path?

Compiling from Test_gregorio_dev

Works. tmp-gre create as expected.

Compiling from alongside

No scores found. tmp-gre not created.

Compiling from subdir

Scores not found. tmp-gre not created

Changing to \gresetgregpath{{../../}} gets the subdir test working with tmp-gre created as expected.

@davidweichiang
Copy link
Contributor Author

Are we ending up with an empty path?

Oh, this is happening when kpse.find_file can’t find the file, so it returns nil, and lfs.normalize is called on nil. Should be an easy fix. But I am not sure why it’s not finding the file. I will have to understand \gresetgregpath better.

@rpspringuel
Copy link
Contributor

\gresetgregpath should work exactly like \graphicspath (from the graphics family of packages), except that it changes \input@path when processing a score rather than when processing an image.

Sent with GitHawk

@davidweichiang
Copy link
Contributor Author

Does the last commit change anything? It was certainly a bug, but I don't understand why the bug was activated in that particular case.

@rpspringuel
Copy link
Contributor

Changing paths and setting \gresetgregpath{{../}}

Compiling from main

Works. tmp-gre has expected structure.

Compiling from Test_gregorio_dev

Works. tmp-gre has expected structure.

Compiling from alongside

Works. tmp-gre has expected structure.

Compiling from subdir

No scores found. tmp-gre created with expected structure (but has no files in it). Changing to \gresetgregpath{{../../}} makes this example work.

Those are all the results I would expect. Barring any other objections I think this is ready for merge. That done I'll upload a new version as a release candidate and we'll hopefully be able to wrap the release in the next couple of days.

@eschwab
Copy link
Contributor

eschwab commented Feb 25, 2025

Looks good to me.

@rpspringuel rpspringuel merged commit 7b3b1c1 into gregorio-project:release-6.1 Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants