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

Make masterFilename what it says on the tin #1835

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

alerque
Copy link
Member

@alerque alerque commented Jul 12, 2023

Closes #1833.

As far as SILE itself is concerned this shouldn't be noticed, but that's not to say documents or other project code that generates files and assumes that masterFilename is a basename not a file name won't be affected. I had in mind to put this in the next breaking release just to be safe, but given the current situation I'm not sure adding more hacks to work around this silly problem in the short term knowing we want to fix it properly anyway makes sense either.

This will break CaSILE but I'll fix that. I don't know if any other 3rd party extensions will be affected.

@Omikhleia does this change make sense to you?

@alerque alerque added this to the v0.14.11 milestone Jul 12, 2023
@alerque alerque added the bug Software bug issue label Jul 12, 2023
Process the basename when that's specifically what we need rather than
making it a basename initially and not having the extension information
when we need it.
@Omikhleia
Copy link
Member

Omikhleia commented Jul 12, 2023

This will break CaSILE but I'll fix that. I don't know if any other 3rd party extensions will be affected.

@Omikhleia does this change make sense to you?

Somehow makes sense, though breaking an internal compatibility might be best in 0.15 rather than 0.14.x... (Especially for something that should occur that often)

This will break too on my side of things. Hoping this quick check caught them all:

find *.sile -name "*.lua" | xargs -d\\n grep masterFilename
embedders.sile/packages/embedders/init.lua:  local dir = pl.path.join(pl.path.dirname(SILE.masterFilename), "embedded")
embedders.sile/packages/embedders/init.lua:  local source = pl.path.basename(SILE.masterFilename)
labelrefs.sile/packages/labelrefs/init.lua:  local tocfile, err = io.open(SILE.masterFilename .. '.ref', "w")
labelrefs.sile/packages/labelrefs/init.lua:  local reffile,_ = io.open(SILE.masterFilename .. '.ref')
printoptions.sile/packages/printoptions/init.lua:  local dir = pl.path.join(pl.path.dirname(SILE.masterFilename), "converted")
resilient.sile/packages/resilient/styles/init.lua:  local fname = SILE.masterFilename .. '-styles.yml'
resilient.sile/packages/resilient/styles/init.lua:  local fname = SILE.masterFilename .. '-styles.yml'
resilient.sile/packages/resilient/tableofcontents/init.lua:  local tocfile, err = io.open(SILE.masterFilename .. '.toc', "w")
resilient.sile/packages/resilient/tableofcontents/init.lua:  local tocfile, _ = io.open(SILE.masterFilename .. '.toc')

Those using the directory where is located the master file should be safe, so I assume the list of impacts is a bit smaller:

embedders.sile/packages/embedders/init.lua:  local source = pl.path.basename(SILE.masterFilename)
labelrefs.sile/packages/labelrefs/init.lua:  local tocfile, err = io.open(SILE.masterFilename .. '.ref', "w")
labelrefs.sile/packages/labelrefs/init.lua:  local reffile,_ = io.open(SILE.masterFilename .. '.ref')
resilient.sile/packages/resilient/styles/init.lua:  local fname = SILE.masterFilename .. '-styles.yml'
resilient.sile/packages/resilient/styles/init.lua:  local fname = SILE.masterFilename .. '-styles.yml'
resilient.sile/packages/resilient/tableofcontents/init.lua:  local tocfile, err = io.open(SILE.masterFilename .. '.toc', "w")
resilient.sile/packages/resilient/tableofcontents/init.lua:  local tocfile, _ = io.open(SILE.masterFilename .. '.toc')

... And adding pl.path.splitext there should be sufficient and compatible with earlier versions decently enough (well, that is unless the master file name contains several dots, but that's really an edge case).

@alerque
Copy link
Member Author

alerque commented Jul 12, 2023

What if we left SILE.masterFilename as a basename for now with a deprecation notice until v0.15.0, but switched SILE's own internal use from the public API to a new undocumented private version (say SILE._masterFilename) that is actually the filename? Would that sit better with you? That would fix our internal bug temporarily without breaking the external API for now. Then later we just swap them out and drop the legacy one.

@Omikhleia
Copy link
Member

Omikhleia commented Jul 12, 2023

Then later we just swap them out and drop the legacy one.

That's extra work later for 3rd party package maintainers.
Maybe we can avoid the need to swap

  • SILE.masterFileName = name with extension, and we use that where needed for now on
  • SILE.masterBaseName = for convenience name without extension (and also to save a few cumbersome calls here and there to pl.path.splitext)

Later, 0.15 or even beyond, we remove the old SILE.masterFilename without extension, so it gets nil.

In the meantime, 3rd party packages can start replacing SILE.masterFilename by SILE.masterBaseName or SILE.masterFilename and there's no urge to remove that shim until it has reached all users having updated everything (SILE, main 3rd party packages, their possible deps, etc.)

Or something similar?

@alerque
Copy link
Member Author

alerque commented Aug 20, 2023

Okay I'll leave masterFilename alone for now so anything 3rd party using it won't trip up, but I don't like any of the suggestions for working around it. As such I've just gone to using input.filenames[1] internally for everything we need it for. Probably we can go back to providing something sensible in a ready-made variable at some point, but I want to cut down on the number of things a user has to input by hand to use SILE as a library that the CLI was just guessing at. That means less magic assumptions but also not asking for things we don't even need.

Where and how we name intermediary files needs a bigger overhaul anyway. It really should be possible to use a cache dir somewhere separate from sources OR outputs, and right now that isn't really possible.

@alerque alerque merged commit f473d20 into sile-typesetter:master Aug 22, 2023
12 of 14 checks passed
@alerque alerque deleted the correct-master-filename branch August 22, 2023 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Software bug issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SILE 0.14.10 cannot properly include a file with same base name as the master file.
2 participants