PreTeXtBook / pretext

PreTeXt: an authoring and publishing system for scholarly documents
https://pretextbook.org
Other
254 stars 203 forks source link

`manage_directories()` feature, implement in LaTeX, req. 3.8 #2088

Closed StevenClontz closed 7 months ago

StevenClontz commented 7 months ago

For @oscarlevin's review first

oscarlevin commented 7 months ago

This is a nice improvement. Note that the pyMuPDF improvements also require 3.8. With 3.12 being released in a few days, I think it is reasonable that people can upgrade to a version that is 4 releases behind, not 5. Plus with virtual environments, does anyone need the older version?

StevenClontz commented 7 months ago

This closes https://github.com/PreTeXtBook/pretext-cli/issues/619 - what happened is that when we did the 2.0 CLI refactor, we forgot to manually manage directories for LaTeX builds, since core never did this.

So, we could go back to doing that, but given momentum to upgrade core requirements to 3.8 anyway, this seems like a good time to upgrade directory management across core pretext.py to use shutil.copytree, and copy over assets where LaTeX can find them even for a simple LaTeX build.

StevenClontz commented 7 months ago

Plus with virtual environments, does anyone need the older version?

I'd argue to consider even requiring 3.9 or 3.10 for a few odds and ends. But CLI users already require 3.8 at least, so there's a bigger jump from 3.8 to 3.9 than 3.6 to 3.8 in some sense.

rbeezer commented 7 months ago

I like the consolidation of all these similar stanzas into one.

Did you like the careful comments about distutils? ;-)

And I'm OK with a move to 3.8, but some warnings will need updating first.

The latex() function is only intended to make the LaTeX file, for use by developers for testing. The pdf() function makes an output format. Copying all the images, etc, will make it more difficult to test changes in the LaTeX file when testing changes in the code.

We should not be encouraging authors and publishers to mess around with the LaTeX file directly. Well, maybe they need to send it to a journal or a commercial publisher. So putting the LaTeX file and the supporting files all in-place, and then making a zip file of a self-contained package could be a useful new function (or variant of existing), as I think we are doing elsewhere in certain cases.

simple LaTeX build

Yes, lets keep it simple. Adding in the images is making something else.

oscarlevin commented 7 months ago

I don't understand how build a latex file that cannot be compiled makes anything simpler. Unless you are very careful about copying the output into a place where it will find the managed assets, or move the managed assets where they need to go anyway, when you try to test changes to the latex file, the build will fail.

Note that any publisher who wants to create a print book will need to mess with the latex. It is also very handy to try to compile the latex separately for finding errors in your authored book. So I think there are more use cases for a latex output format than just development.

rbeezer commented 7 months ago

You make the LaTeX file, you appl;y a PR, you make the L:aTeX file again. Then you do a diff on the two files to see if you get what you expect. That is the purpose of the latex() function. You don't want piles of other files around.

We can do new things, but this function should not change.

StevenClontz commented 7 months ago

I've added a keyword argument that defaults to current behavior, but lets the user request management of asset directories. Would this satisfy everyone?

rbeezer commented 7 months ago

That is fine. But I'd like it to be a positional argument (that is intentional) modeled closely on the file_format argument of html() (which is where distutils went away).

And then make it a zip file which an author can take and do what they want with. I am tired of people thinking they need to adjust the LaTeX output and therefore we have bugs, when they are not really configuring things properly in the first place (not the case in provoking incident this time). So, no, I do not want to make it easy to get an "in-place" ready-to-compile LateX tree. Compilation is tested with the pdf() function.

StevenClontz commented 7 months ago

Do you want the new manage_directories function to use all positional arguments as well?

Supporting ZIP output for LaTeX builds is outside the scope of this PR but my arm can be twisted to take a crack at it in a separate contribution.

Actually, I'm rather sympathetic to disemphasizing building LaTeX directly. I had slyly removed LaTeX as a format included in the 2.0 default project.ptx, but someone else added it back and I felt it wasn't worth the time to discuss.

So given these specifications, my inclination is to remove use_manage_directories as an argument for latex() altogether, and make a decision on the CLI side for best behavior for managing directories in our latex builds. Later, when latex() supports file_format="zip" it probably makes sense to manage directories as part of the ZIP procedure. Sound ok Rob?

rbeezer commented 7 months ago

Do you want the new manage_directories function to use all positional arguments as well?

That feels more internal, and not API-ish so the keywords are fine.

I'm rather sympathetic to disemphasizing building LaTeX directly.

Good, we are on the same page.

So given these specifications, my inclination is to remove use_manage_directories as an argument for latex() altogether, and make a decision on the CLI side for best behavior for managing directories in our latex builds. Later, when latex() supports file_format="zip" it probably makes sense to manage directories as part of the ZIP procedure. Sound ok Rob?

Yes, that sounds like a good plan.

Holler when I can start testing... Thanks for all the discussion.

rbeezer commented 7 months ago

Should I make the changes (basically warnings) to move to 3.8 as the minimum Python? (And announce.)

StevenClontz commented 7 months ago

Should I make the changes (basically warnings) to move to 3.8 as the minimum Python? (And announce.)

Sounds fine to me. Maybe note that CLI has required 3.8 for some time now, so the announcement shouldn't affect any CLI users.

StevenClontz commented 7 months ago

This PR is ready for Rob's review.

rbeezer commented 7 months ago

All done - thanks for the good work and the discussion.