PreTeXtBook / pretext-cli

Command line interface for quickly creating, authoring, and building PreTeXt documents.
https://pretextbook.org
GNU General Public License v3.0
17 stars 18 forks source link

Behavior change for pretext.xsltproc() #34

Closed rbeezer closed 4 years ago

rbeezer commented 4 years ago

In the PreTeXt pretext.py module, I need to make it clear that the filename for the publisher file needs to enter the function as a full path and therefore on the pretext command-line it needs to be specified on the command-line relative to the current directory.

This could be a small change in behavior, so I thought I'd put this here. Close quickly if it is not relevant.

oscarlevin commented 4 years ago

Good to know. I don't remember exactly what we have, although I've started using os.path.abspath() around everything. I'll leave this open just to remind me to double check that whatever you do still works, or else make the minor change needed.

rbeezer commented 4 years ago

Yes, I'm learning the hard way to be religious about santitizing and normalizing user input at the very first opportunity, instead of chasing it around later. .abspath() is key.

StevenClontz commented 4 years ago

I wonder if there's a way to extend/configure click.Path() to always convert user-provided paths into absolute paths.

Steven Clontz https://clontz.org - steven.clontz@gmail.com

On Fri, Jun 26, 2020 at 12:29 PM Rob Beezer notifications@github.com wrote:

Yes, I'm learning the hard way to be religious about santitizing and normalizing user input at the very first opportunity, instead of chasing it around later. .abspath() is key.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/StevenClontz/pretext.py/issues/34#issuecomment-650303005, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL4YUDD3ACA5QFYDMCIS6TRYTLGZANCNFSM4OHXE6AA .

oscarlevin commented 4 years ago

Indeed. Just found resolve_path option: https://click.palletsprojects.com/en/7.x/api/#click.Path.

When this goes to a config file, we will need to be careful with that.

oscarlevin commented 4 years ago

I'm at a loss. To match the core pretext behavior, we should specify the path to the publisher file based on the current working directory. We can convert to absolute paths using either Click's resolve_paths=True option for click.Path(), or the standard os.path.abspath(). However, when we do this, at least on Windows, the stringparam for 'publisher' becomes something like "C:\\Users\\oscar.levin\\Documents\\GitHub\\scratch\\my-book\\pub-standard.xml'. This is the path I expect, but it does not get passed to xsltproc correctly. In fact, whether or not this publisher file is present at that location, we get no warning or no effect.

Compare to the situation where we don't enforce an absolute path. Then 'publisher' is set to 'pub-standard.xml', and if 'pub-standard.xml' is not in the same directory as the main.ptx file, we get an lxml error message. (We should exit with a better error message, but that's another issue).

So there is something I don't understand about how the publisher stringparam is being passed to the stylesheet. Any ideas?

rbeezer commented 4 years ago

This might help, or it might be noise.

*OR, the path can be absolute.

For the pretext/pretext script I form absolute paths just as soon as I possibly can, before code starts changing directories. I know this change was provoked by some bad behavior on Windows (but I forget exactly).

oscarlevin commented 4 years ago

Could it be that the xsl expects forward slashes and doesn't know what to do with backslashes?

Evidence:

Now the issue is that os.path.abspath() returns backslashes on Windows. So for one thing, I don't know what to do to fix this, and second, I'm worried that the core pretext script might also be broken. @rbeezer, is there a call to the pretext core script that actually uses the publisher file that I could test on?

rbeezer commented 4 years ago

Well, i started down this road back in May to eliminate all the Windows-specific code that messed with backslashes. ;-) And rounded up as much Windows testing as I could muster. Which is not to say it is correct. But I think os.path.join() handles all this intelligently. You are using that, no?

pretext/pretext -c all -f html -p my-pub-file.xml mybook.xml

will pass along the publisher file. I think you can chase that through to see what sort of test to run.

oscarlevin commented 4 years ago

Aha! Yup, pretext core is broken too. No error, but it does not actually process the publisher file.

I suppose we could do a string replace on the slashes just for the publisher file? It seems like this is an xsl issue, not python.

oscarlevin commented 4 years ago

Replacing slashes before sending params to xsltproc works. Pull request for mathbook incoming.

rbeezer commented 4 years ago

See def get_platform() in 41efe0010fe9880059da4397ba66b83aeb37008f

Condition the change on being on Windows?

oscarlevin commented 4 years ago

Fixed at 7307392024b5b75b56d5d58170dc9a8abd4f0f9a