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

Handle documents on STDIN and/or via Lua 5.1 #69

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Dec 7, 2023

I noticed while checking my suggestions in this comment that resilient is tripping up on documents coming from STDIN, e.g.:

$ sile -o test.pdf - <<< "\\document[class=resilient.book]{test}"
SILE v0.14.13 (LuaJIT 2.1.1696562864)
<STDIN> as sil

! Unexpected Lua error

error summary:
        Processing at: STDIN: in <snippet>:
                [[\document[class=resilient.book]{test}␤]]
        Using code at: ...es/share/lua/5.1/sile/packages/resilient/styles/init.lua:91: attempt to concatenate field 'masterFilename' (a nil value)

Run with --traceback for more detailed trace leading up to errors.

This should fix that by doing the same thing SILE is internally, but I haven't actually tested it.

@@ -88,7 +88,8 @@ end

function package:readStyles ()
local yaml = require("resilient-tinyyaml")
local fname = SILE.masterFilename .. '-styles.yml'
local basename = pl.path.splitext(pl.path.normpath(SILE.input.filenames[1]))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So somehow the magic behind SILE.masterFilename got broken and 3rd party packages have to use an even weirder magic pl.path.splitext(pl.path.normpath(SILE.input.filenames[1]))?

I suggest reporting the issue to SILE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it didn't "get broken." It was always broken because it made assumptions about inputs that limited usage. You ran into one such issue yourself in SILE#1833. In working on a fix I ran into several other issues and also raised the point that masterFilename is not even a file name at all (c.f. SILE#1835). It also has never properly handled STDIN or named pipes (something we added in SILE v0.9.5) because it can't invent a name, and more recently it became obvious that it doesn't play nice with multiple inputs.

We disagreed on what the most useful abstraction was and what to call it, so rather than remove the poorly named and partially functional API we've just left it for now but started using more robust code internally. There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.

Normalizing a path and getting the base name is not exactly weird magic, but this code is making assumptions about projects using certain file system layouts (and even using real files instead of named pipes, anonymous streams, command substitution, etc.). It's okay to make those assumptions for this use case, but SILE can't magically do the right thing in every case so explicitly doing exactly the thing you want to accomplish here makes sense to me.

Copy link
Owner

@Omikhleia Omikhleia Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was always that 3rd party packages should not have to do pl.path.splitext(pl.path.normpath(SILE.input.filenames[1])) for deriving in a (fairly obscure way) where SILE should have its output.

In sile-typesetter/sile#1835 (comment) you said:

Okay I'll leave masterFilename alone for now so anything 3rd party using it won't trip up.

Still, it now bugs on attempt to concatenate field 'masterFilename' (a nil value)

There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.

With 15 sile modules on Luarocks, yes there is a reason... I doubt I can cope fast enough with an impredictible release cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was always that 3rd party packages should not have to do pl.path.splitext(pl.path.normpath(SILE.input.filenames[1])) for deriving in a (fairly obscure way) where SILE should have its output.

You don't have to normalize the path if you don't want to, but I don't understand what you think should happen here. I offered to supply something like a SILE.masterBaseName or something like that and I'm still happy to do so, but I also struggle to see why this seems so hard. Sile gives you the list of inputs as SILE.input.filenames. If you want to guess at a meta data file that might exist with a similar name, removing the .sil extension from it with :gsum() or pl.path.splitext() is not exactly magic. For many of my use cases that isn't even what I do because I don't want my project directories cluttered up with build artifacts, with CaSILE I place things like this in a completely separate build cache directory with other intermediate artifacts. Maybe your system doesn't involve stripping extensions at all, maybe you just append another suffix.

I guess my point is SILE.input.filenames[1] seems like a pretty straight forward way to find the first input filename. If you want to derive something from that, go ahead.

In sile-typesetter/sile#1835 (comment) you said:

Okay I'll leave masterFilename alone for now so anything 3rd party using it won't trip up.

Still, it now bugs on attempt to concatenate field 'masterFilename' (a nil value)

No it doesn't "now bug" ... it still works just the way it has since 0.9.x days which was always incomplete. Your module is inventing a file name and attempting to read it. It's deriving that name from what it assumes will be an input filename, but it never accounted for handling streams where there is no master file name because the input isn't a file.

You've never run into this before probably because you haven't worked in the shell with pipes, and even if you had Lua 5.4 was masking this bug from you by silently casting Nil to a string. Using Lua 5.1 / LuaJIT happens to reveal this bug because it doesn't do that cast and hence shows you when you can't guess a filename based on something that doesn't exist.

There are all sorts of use cases where SILE can be used to render that don't involve either input files or even output files. As a contrived example, lets say you wanted to render a text string to an image and place it here in GitHub as a Markdown image. Running this command:

$ echo -E '\document{\nofolios{}Foo Bar}' | sile -q - -o - | magick -density 300 - -trim png:- | wl-copy

... and then hitting Ctrl+V results in this:

image

There is no reason for 3rd party packages to not adopt this until such a time a a standardized base name function of some kind is provided that covers all these bases.

With 15 sile modules on Luarocks, yes there is a reason... I doubt I can cope fast enough with an impredictible release cycle.

I don't expect you to cope with everything. This is open source and when I saw a problem that came up in my own usage I submitted a fix for you. If you want to suggest what API SILE should provide I'm happy to go that route, I just didn't want to put work into adding fixes to SILE.masterFilename when it wasn't even a file name and the actual filename was in SILE.input.filenames. Again see the related SILE PR. I didn't break anything about masterFilename yet, only suggested it should be superseded by something more correct if we keep anything at all.

@alerque alerque changed the title Handle documents on STDIN in SILE v0.14.11 Handle documents on STDIN and/or via Lua 5.1 Dec 8, 2023
@alerque
Copy link
Contributor Author

alerque commented Dec 8, 2023

I dropped the path normalization function which isn't necessary here and also changed the PR title because my reference to a SILE version seems to have thrown off the discussion. This is not a problem that started with SILE v0.14.11, it only fixes a bug in this library by making use of the more consistent SILE.input array provided in more recent SILE versions, a fix that would not have been quite so easier in earlier releases.

@Omikhleia
Copy link
Owner

Omikhleia commented Dec 8, 2023

Possibly affected modules (using masterFilename)

@Omikhleia
Copy link
Owner

Omikhleia commented Dec 8, 2023

The concern with embedders, printoptions, labelrefs and the ToC is a bit different from that of resilient styles

  • The former need a temporary file or folder to store things for next re-runs, and are usually not committed with the project
  • Styles are generated upon first run, and only "augmented" afterwards (for potentially missing styles, but not changing existing ones), and are manually tuned etc. to the user's convenience and are usually committed with the project.

@Omikhleia
Copy link
Owner

Hmm. In the above sense, styles are perhaps incompatible with content from STDIN. Perhaps this workflow should just be rejected...

@Omikhleia Omikhleia marked this pull request as draft December 8, 2023 23:32
@alerque
Copy link
Contributor Author

alerque commented Dec 8, 2023

These classes seem to work fine when the style or other possible external files are not found. I would still make this change and just let it not find the appropriate file and skip it rather than crashing on not being able to guess a filename.

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