pjheslin / diogenes

Diogenes: an environment for reading Latin and Greek
https://d.iogen.es/d
Other
56 stars 10 forks source link

File::Spec->catpath ignoring tlg_dir on macOS #80

Closed ben-larson closed 2 years ago

ben-larson commented 2 years ago

When I search for an author in TLG Texts corpus, I get an error page.

image

Steps to reproduce:

What I think the problem is

Disclaimer: not a perl developer

There was a commit called Fix chron search filename paths on Windows. (https://github.com/pjheslin/diogenes/commit/e1126a4c200f03fe0c4dcfae276a89700b4e2f7b) which removed $vol from a call to File::Spec->catpath. I think this broke unix systems, because the [documentation](https://perldoc.perl.org/File::Spec#catpath()) for File::Spec says:

Under Unix, $volume is ignored, and directory and file are concatenated.

I think it expects the volume to be the first of three arguments, so it just ignores the first one on unix systems. Based on my logging, only the file name is being returned by catpath.

File::Spec->catpath($self->{tlg_dir}, $filename); # tlg9007.txt
File::Spec->catpath($vol, $self->{tlg_dir}, $filename); # /Users/benlarson/Documents/TLG-E/tlg7052.txt

When I put $vol back, it works.

Please let me know if there's more context I can provide, or if I'm just confused and this is just due to misconfiguration on my end.

ben-larson commented 2 years ago

Using an empty string as the volume works on macOS, so I put in pull request #81 for that. I do not have access to Linux or Windows systems to test it out, so I left it as a draft pull request, but if it works on those then it is ready for review.

pjheslin commented 2 years ago

Thanks for the fantastic bug report and fix! I'm sorry it has taken me so long to get back to you -- it has been a very busy university term.

I was fixing a problem with Windows users where the volume appeared in the path twice. Removing the volume from the call to catpath fixed it on Windows, but broke it on Mac. Your fix is correct and I have merged it. I should have supplied an empty volume instead.

The reason I did not catch this before is that it typically does not break searching the TLG; it just breaks chronological searching, which is a very new feature. I'm not sure why it broke things more badly for you.

Note that the -P argument for the server is obsolete and was never used to indicate the location of the TLG. It was used once upon a time for telling the server where the Perseus data was located.