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

--paper option ignored #78

Closed
compwiztobe opened this issue Apr 24, 2024 · 40 comments
Closed

--paper option ignored #78

compwiztobe opened this issue Apr 24, 2024 · 40 comments

Comments

@compwiztobe
Copy link

pdfjam version: 3.10 (arch linux, texlive-binextra 2024.2-1)

Is it just me or is the --paper option (both --paper a4paper and --a4paper) completely ignored? As well as /etc/papersize as suggested by libpaper..

The only way I can get pdfjam to output A4 sized pages instead of defaulting to letter (even with A4 sized inputs!!) is with

--papersize '{297.1mm,210mm}'

which is obviously not ideal.

What am I missing?

@DavidFirth
Copy link
Collaborator

Indeed that seems not to work with the current version of pdfjam (as far as I can tell from a quick try here with 3.10).

I am still using pdfjam 3.03 on my computer. The --paper and --a4paper options work fine in that version. I have not tested any other versions than 3.03 and 3.10 for this.

So something has got broken between 3.03 and 3.10 it seems. (likely due to one of the "enhancements" that people seem to insist on suggesting?)

@rrthomas
Copy link
Owner

Thanks for checking, @DavidFirth. I'm embarrassed, as this has happened on my watch, so it seems I've been too happy to accept additions.

So, I've done some testing. With current git master, if I run

$ pdfjam a4-1.pdf
…
$ pdfinfo a4-1-pdfjam.pdf
…
Page size:       595.276 x 841.89 pts (A4)

I also tried with --paper a4paper and it worked.

In other words, it's working in this case. @DavidFirth, if you can give me a reproducer that fails, I can look into it further.

@DavidFirth
Copy link
Collaborator

Thanks, and please don't feel embarrassed, we're all just doing our bit (and you are doing more than most!)

I wonder if it is really working at your end though @rrthomas --- maybe like me you have A4 set as a default somewhere? Have you tried it with the option --a5paper for example?

@rrthomas
Copy link
Owner

Still working:

$ pdfjam --papersize a5paper a4-1.pdf
…
$ pdfinfo a4-1-pdfjam.pdf
…
Page size:       419.528 x 595.276 pts

@DavidFirth
Copy link
Collaborator

DavidFirth commented Apr 25, 2024

That's odd. As far as I can tell (from pdfjam --help), using --papersize a5paper should result in an error. And it does that for me, here (with the current github master):

david@DF-thinkpad:/data/david/temp/pdfjam$ ./bin/pdfjam /data/david/temp/1142362.pdf --vanilla --papersize a5paper -o papersize-a5paper.pdf
          ----
  pdfjam: This is pdfjam version N.NN.
  pdfjam: Called with '--vanilla': no user or site configuration
          files will be read.
  pdfjam: Effective call for this run of pdfjam:
          ./bin/pdfjam --vanilla --papersize 'a5paper' --outfile papersize-a5paper.pdf -- /data/david/temp/1142362.pdf - 
  pdfjam: Calling /usr/bin/pdflatex...
  pdfjam: FAILED.
          The call to /usr/bin/pdflatex resulted in an error.
          If '--no-tidy' was used, you can examine the
          log file at
                  /tmp/pdfjam-Z8316C/a.log
          to try to diagnose the problem.
  pdfjam ERROR: Run 1: Output file not written

I used the following to attempt A5 output as documented:

./bin/pdfjam /data/david/temp/1142362.pdf --vanilla --a5paper -o a5paper.pdf
./bin/pdfjam /data/david/temp/1142362.pdf --vanilla --paper a5paper -o paper-a5paper.pdf

No error message. But the resulting output is A4 in both cases (as given by pdfinfo).

So this is all very puzzling.

In case it's relevant, here is my locale data:

david@DF-thinkpad:/data/david/temp/pdfjam$ locale
...
LC_PAPER="en_GB.UTF-8"

@lemniscati
Copy link
Contributor

A last --papersize option overrides any --papersize options preceding it, if any,
AND it ALSO overrides any --paper or --*paper options even if they are succeeding it,

The reason is that $paper is specified as an option of documentclass while $papersize is specified as an option of geometry package.

After commit b615592, default paper/papersize will be set as if a --papersize option is specfied WHEN paper command EXISTS.
So, it can happen that any --paper or --*paper options are ignored when paper command exists.

This might be a cause of the issue.

@compwiztobe
Copy link
Author

A few things:

Just want to clarify that my system shows

$ paper
Letter: 8.5x11 in

based on my locale en_US.UTF-8, and I had found this to be the source of the behavior above (also the same reason /etc/papersize is ignored, an issue for libpaper maintainers...). This locale setting was never a problem in my recent memory, perhaps from 3.03 and earlier. (I already override it for other particular purposes like ISO date formatting, but this gives me another reason to abandon the motherlocale entirely.)

I'm not sure why this change b615592 was made. I had noticed that in cursory viewing of the pdfjam script as well and wondered if that could be possibly be what was happening... Options provided on the command line must always override anything else specified by other libraries or the environment.

Looking more closely the problem seems to be that this commit sets a default --papersize option from the paper command, which can be overriden on the commandline, but --papersize always overrides --paper option. So providing --paper option at command line does nothing if paper command exists, only overriding with --papersize replaces the value given by paper.

Definitely clarify if you are getting working behavior with --papersize a5paper or --paper a5paper. --papersize should only accept a size pair '{$width$unit,$height$unit}' (or the other way not sure..) according to the docs.

@DavidFirth
Copy link
Collaborator

So that commit introduces a bug, if I understand correctly. (a bug in the sense that behaviour is no longer as documented) Simplest fix would be to revert it? Maybe there's a better way but I don't know it.

@compwiztobe
Copy link
Author

Issue summary:

If paper command (from libpaper) exists, then default --papersize value is added from that default size. This option overrides any --paper option on the command-line, and can only be overridden with another --papersize option.

@compwiztobe
Copy link
Author

Suggested fix:

Only populate --papersize option with this default value if it exists and --papersize is not on the command-line AND --paper is not provided on the command-line.

It would also be helpful if pdfjam --help listed the default value it was going to use in the --paper option section, currently this is only populated from the $paper env var, which is not even the variable used by the paper command (that's $PAPERSIZE).

@DavidFirth
Copy link
Collaborator

Indeed it used to be the case that pdfjam --help listed the default value that would be used for the --paper option. I do not know why that was removed.

As for the suggested fix: care would be needed to make sure it agrees with the documentation. What if the user or sysadmin has specified a default papersize for pdfjam in a configuration file for example, as documented in pdfjam --help?

@DavidFirth
Copy link
Collaborator

It should be noted that pdfjam is maintained by @rrthomas only temporarily, until someone else volunteers to take it over. There's a currently pinned issue about this. At this point, then, I think that only very simple fixes ought to be considered, which don't demand much checking.

@compwiztobe
Copy link
Author

compwiztobe commented Apr 25, 2024

If I had to guess the help text regression was introduced by a much older change, 80f7e4c, part of #8.

Help text printf was moved up before any of the configs are read in.

@lemniscati
Copy link
Contributor

lemniscati commented Apr 25, 2024

Yet another suggested fix:

When processing options --paper, --*paper, or --papersize, always make BOTH variables $paper and $papersize be updated properly. That is, always treat $paper and $papersize as a pair/union of parameters:

  • --paper, or --*paper updates $paper, and clears $papersize.
  • --papersize clears $paper, and updates $papersize.

@rrthomas
Copy link
Owner

I will look at this. Thanks for all your analysis and suggestions, I am confident there's a sensible story to pull together and implement in there when I have a moment to sit down and look at it properly.

@compwiztobe
Copy link
Author

compwiztobe commented Apr 25, 2024

While --papersize on the command line may override --paper on the command line, the principle should be that any --papersize or --paper default given by libpaper, env vars, or config files, should not override anything given on the command line. As you've mentioned care should be taken to ensure the config file parsing respects this.

Simply setting the $paper and/or $papersize variables in the script with the defaults doesn't do this, since only one of them might be provided on the command line and that should take precedence.

@lemniscati
Copy link
Contributor

This (lemniscati@f90b102) is a modification for #78 (comment): Make options --paper, --*paper and --papersize override preceding ones.

But it is not for modification of pdfjam-help.txt.

@lemniscati
Copy link
Contributor

By the way, according to the current sample pdfjam.conf, papersize is not supported in configuration scripts:
papersize is not contained in pdfjam.conf, and is excluded from configurable parameters as written in the last line

##  END OF FILE: that's all you can configure here!

@rrthomas
Copy link
Owner

OK, I think there are two issues to deal with here:

  1. Default paper size from paper incorrectly overriding command-line options and user settings (mea culpa).
  2. Lack of display of current configuration in --help. I'll look into this next. By the way, I don't propose to allow papersize being used in configuration files—I think paper should be used for the default setting, and by now it's available to install on most systems. (If I were doing serious work on pdfjam, I would want to simplify this area!)

@rrthomas
Copy link
Owner

So, about the configuration, I see several references to environment variables such as paper above. But nowhere is this documented, that I can see. In other words, I do not expect setting paper in the environment to have any effect on pdfjam (if it does, it's a bug). So I do not propose to make that work. Indeed, currently a setting of paper should have no effect in any case, because either it is set to the empty string if the paper program is installed, or it is set to a4paper as a fallback default.

This means that configuration of paper size has only three sources: first, paper's default (if available, or a4paper if not); and next the --paper and --papersize command-line options. So the bug is that currently, if the paper program is installed, then it always sets papersize (this is because paper knows about many more paper sizes than geometry, and the user can configure custom sizes), and this overrides any command-line --paper option.

So in summary, I think fix for this bug is very simple: command-line options should override previous settings. In particular, using --paper on the command line should clear the current --papersize setting, and vice versa.

So in fact, I am in complete agreement with @lemniscati's solution. @lemniscati, please could you make it a PR?

@lemniscati
Copy link
Contributor

@rrthomas, all right. I've made a PR #79.

@rrthomas
Copy link
Owner

I have merged the PR, and also pushed fixes for the output of --help. I would appreciate feedback before I make a release. I have made two main changes:

  1. Move the evaluation of helptext and the interpretation of --help later in the file again, after the configuration has been read, but before the command-line has been processed.
  2. Add the default value of --papersize to the output. It is unfortunate that with paper installed, the "default default" is a --papersize setting, not a --paper setting. This is because, as mentioned above, we cannot assume that the geometry package will understand a paper name from paper. (The solution I'd prefer is to make paper mandatory, but that would be inconvenient for some users.)

@lemniscati
Copy link
Contributor

lemniscati commented Apr 28, 2024

About 1. It looks ok.

About 2. (1) libpaper2 seems relatively new and there are some systems which does not adopted yet. I would not like it to be mandatory.
About 2. (2) A command line option --papersize 'default defalut' may bring an error unless it is overridden in some proper way. So, I'm afraid that, with paper command, we are obliged to specify one of valid --paper, --*paper, and --papersize options explicitly (not tested because I don't have paper command). So, I guess, outputs from paper command should be treated properly, so that they bring no error. (But analysis of output from paper looks complicated... It would be convenient if there were simple machine-readable outputs of paper...)

@rrthomas
Copy link
Owner

A command line option --papersize 'default defalut' may bring an error unless it is overridden in some proper way.

Can you explain this please? How would such a command-line option arise?

@lemniscati
Copy link
Contributor

lemniscati commented Apr 28, 2024

  1. Add the default value of --papersize to the output. It is unfortunate that with paper installed, the "default default" is a --papersize setting, not a --paper setting.

'default default' comes from here ↑

Ah, sorry...
I misunderstood completely...

@DavidFirth
Copy link
Collaborator

DavidFirth commented Apr 28, 2024

Thanks for doing all this. I can aim to do some checking in the next few days, if that would be helpful. I wonder, @rrthomas can you make a release file (ie, as it would be made based on the current master) and send it to me? Then I would be able to check it all, including checking that pdfjam --help behaves as expected after the package has been built.

@rrthomas
Copy link
Owner

rrthomas commented Apr 28, 2024

@DavidFirth I've sent over a build archive.

@DavidFirth
Copy link
Collaborator

pdfjam-findings.md
Many thanks @rrthomas for sending the pre-release zip file. As promised I did some testing. It's far from exhaustive, but hopefully it's enough for now.

I attach a file here with the results of my checking --- I hope it's all clear enough. I have listed everything, so that you can see what worked as well as some things that did not. The problems I found are labelled BUG1...BUG6. There's also a question at QUERY1. It's not as bad as it looks: BUG4-BUG6 probably all have the same root cause (ie, an unnecessary call to paper when a paper= setting has already been made in a config file). BUG1 is I think about the --landscape option not playing well with a papersize setting: this needs to be resolved somehow (and then documented I suppose). BUG2 is perhaps less of a priority unless it's easy to fix (it relates to the possibility that paper from libpaper is masked by another executable in the PATH). BUG3 is about slightly ambiguous documentation of the --vanilla option, and seems easy to fix once it's decided how --vanilla should affect (or not) a possible call to libpaper's paper.

I had intended in addition to do some checking of the --papersize option, including what happens when geometry.sty is missing (about which there is a specific statement in pdfjam --help). But I ran out of time for that, sorry.

I hope it helps!

I haven't looked into the code at all (to be honest, I was never expert in shell scripting and I know even less about it now). I hope that it's not too hard to fix the problems I found. If you need any more checking from me, do let me know.

@rrthomas
Copy link
Owner

Thanks for this, @DavidFirth. I'll have a look when I can.

@rrthomas
Copy link
Owner

I have fixed BUG1 (--landscape not working). The problem was that landscape has no effect in geometry if papersize is set (at least, as far as I can see); the fix is to switch the height and width in papersize if --landscape is used.

I have responded to QUERY1 by documenting the ultimate fallback paper size of A4 in --help.

@rrthomas
Copy link
Owner

I have added a sanity check for the output of paper to fix BUG2.

@rrthomas
Copy link
Owner

I have fixed BUG3 by clarifying the documentation for --vanilla.

@rrthomas
Copy link
Owner

rrthomas commented May 1, 2024

I believe I have now fixed the remaining bugs, by adopting your suggestion @DavidFirth.

I would also like to commit your test procedure, if I may. I think it would be quite easy to adapt psutils's test suite to pdfjam—it just runs commands on PDF files and compares the results with expected output—but in the absence of that, a good manual test procedure is something.

@DavidFirth
Copy link
Collaborator

DavidFirth commented May 1, 2024

Thanks, yes that's fine by me. Would you like me to do further testing now before making a release? (it would need to be at the weekend, because I'm away in the next 2 days)

@rrthomas
Copy link
Owner

rrthomas commented May 1, 2024

Yes please!

@DavidFirth
Copy link
Collaborator

I have responded to QUERY1 by documenting the ultimate fallback paper size of A4 in --help.

As it stands in the modified output of pdfjam --help, it looks as though A4 will be used if the --paper option is not used (either on the command line or via a pdfjam config file?). But isn't there the possibility that libpaper will deliver a different default paper size in that case?

Related: I myself am unsure now exactly what takes precedence on the pdfjam command line, is it --paper or --papersize? This ought to be documented I think (and probably would be best dealt with by disallowing the use of both of those options?)

Also related: the output of pdfjam --help now seems to imply that the --papersize option can have a default value set for it. (it says [Default for you at this site: ] --- but I don't think that should be there in the --help output, because it's not possible to set a default for that via the pdfjam config file(s)?

@DavidFirth
Copy link
Collaborator

A few days ago I wrote:

Would you like me to do further testing now before making a release?

I now have to apologise, because other things at present are limiting severely the time I can spend at the computer. I have not been able to any further checking beyond looking at the --help issue mentioned above --- sorry. Sorry I can't be of more use than this.

@rrthomas
Copy link
Owner

rrthomas commented May 5, 2024

No worries, @DavidFirth; I will take your recent comments into account then make a release.

@rrthomas
Copy link
Owner

rrthomas commented May 9, 2024

As it stands in the modified output of pdfjam --help, it looks as though A4 will be used if the --paper option is not used (either on the command line or via a pdfjam config file?). But isn't there the possibility that libpaper will deliver a different default paper size in that case?

There is, hence the phrase "In the absence of any other setting". But I agree that that could be taken to mean "pdfjam setting", so I'll clarify.

Related: I myself am unsure now exactly what takes precedence on the pdfjam command line, is it --paper or --papersize? This ought to be documented I think (and probably would be best dealt with by disallowing the use of both of those options?)

In common with many other command-line programs, last option wins. This allows in particular straightforward programmatic use of pdfjam, where the command line is generated by another program.

Also related: the output of pdfjam --help now seems to imply that the --papersize option can have a default value set for it. (it says [Default for you at this site: ] --- but I don't think that should be there in the --help output, because it's not possible to set a default for that via the pdfjam config file(s)?

It can't be set in the configuration, but it can get a default value from libpaper (recall, this is because libpaper knows about paper sizes that geometry does not, so it sets papersize not paper).

Again, I'll clarify the wording here.

@rrthomas
Copy link
Owner

rrthomas commented May 9, 2024

I have released version 3.11. I will close this issue; of course, there may be others!

@rrthomas rrthomas closed this as completed May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants