llmk icon indicating copy to clipboard operation
llmk copied to clipboard

Add output_directory option to top-level config

Open pineapplemachine opened this issue 1 year ago • 2 comments

This PR should be reviewed and merged after #14 (Support for makeglossaries)

The PR adds support for an output_directory option in top-level config. This is passed to latex via the -output-directory option. Other commands, such as makeindex, need the output directory path to be prepended to the basename.

The %B directive was been changed to a concatenation of the output directory and the basename, in the case that an output directory was provided, in order to facilitate this. The %b directive was added to produce the same behavior as %B did previously. The %o directive was added to insert the output directory path itself, or . in the case that no output directory was provided.

The makeglossaries command required that llmk be able to distinguish between a target path as provided to the command, and the true target path, so that llmk can check if the target file exists or has been updated. This is important because the directory containing *.glo and *.ist files must be provided to makeglossaries via the -d option; simply concatenating the output directory and the *.glo target path will not work. To achieve this, the target_path attribute has been added to program objects. This allows the actual target path to be provided to llmk as a distinct value from the target path provided as the final argument to a makeglossaries command.

pineapplemachine avatar Aug 18 '22 22:08 pineapplemachine

Could you provide a simple example LaTeX document that uses your proposing feature? It can also be added to the examples directory if you want.

This is passed to latex via the -output-directory option.

This will be true only if a user doesn't change the default programs.latex table containing '-output-directory="%o"' in the value for opts; but this can be customized. Since almost every setting in the programs table can be changed, at least by now we don't support any specific option for a specific program including the latex program.

The makeglossaries command required that llmk be able to distinguish between a target path as provided to the command, and the true target path, so that llmk can check if the target file exists or has been updated.

Ok, then maybe we can make an exception for your output_directory proposal, justified by the convenience of the %o specifier and target_path. But we have to make this clear: one can always write either absolute or relative paths directly to the args and opts for each item in the programs table. How %o and target_path are more beneficial than writing the explicit paths directly to the existing keys?

Thank you for also writing the documentation about the feature. Perhaps I also need some explanation about target_path (especially the difference in its behavior from the existing target).

wtsnjp avatar Aug 20 '22 08:08 wtsnjp

Could you provide a simple example LaTeX document that uses your proposing feature? It can also be added to the examples directory if you want.

Ok, I'll add one soon.

One can always write either absolute or relative paths directly to the args and opts for each item in the programs table. How %o and target_path are more beneficial than writing the explicit paths directly to the existing keys?

The reason for this PR is that, to specify an output directory, one would otherwise be required to override every program configuration to account for it. This is information that must be provided in every step of the process (normally as part of the target path), not only to the latex program. Note that the behavior of %B is changed by this PR. It is not only for providing an argument to the latex program.

I would be open to modifying this PR to append an -output-directory option to the latex program even when the program's options were customized (in the case that an output_directory was configured), as well as a -d option to the makeglossaries program. This way, user overrides of the program options would not invalidate the output_directory option.

I would also suggest that it ought to be possible to specify the output directory as a command line argument. But since the current command line parser is set up only to recognize boolean flags, and since I don't currently have a particular need for this feature for my own work, I considered any rewrite the args parser to be out of scope and a possible future task for a different contributor. I bring this up because I think that this would be the best argument for providing an output directory using a dedicated configuration option, and not by overriding every program configuration: It is often very helpful for a build tool such as llmk to accept an output destination. Without internal logic to handle an output directory, like is implemented in this PR, it is very inconvenient to assign a different output directory for any given run of llmk.

Thank you for also writing the documentation about the feature. Perhaps I also need some explanation about target_path (especially the difference in its behavior from the existing target).

If only target is specified, then behavior does not change from before.

If target_path is specified in addition to target, then this overrides llmk's own internal path for determining whether a target file exists or has changed. This is needed because it is not possible to specify an output directory path via the target for makeglossaries. The directory path must be provided via the -d option and the target must be the file's base name.

So where the makeglossaries command may look like this: makeglossaries -d my-output-dir my-doc.glo, the actual target path will be my-output-dir/my-doc.glo. If llmk does not know to check this actual target path, then it will look for the same target path given to makeglossaries, my-doc.glo without the preceding directory, and since no such *.glo file exists it will not proceed with the command. The purpose of target_path is to solve this problem.

pineapplemachine avatar Aug 22 '22 14:08 pineapplemachine

@wtsnjp Can you please help me out with examples_spec.rb L58? I wanted to add a test for the example that uses the output_directory option. Unfortunately I am not experienced with Ruby and I couldn't work out how to create the output directory, either when implementing this feature before (to automatically create it if it didn't exist) or when trying to have this new test create the directory beforehand.

Despite using Dir.mkdir in what seemed like the sensible way, I still got this test failure. Even though, from manually using this added option for my own project, I know that it should work:

Failures:

  1) Processing example outputdirectory.tex should produce outputdirectory.pdf
     Failure/Error: expect(file?('output/outputdirectory.pdf')).to be true
     
       expected true
            got false
     # ./spec/examples_spec.rb:164:in `block (3 levels) in <top (required)>'

Finished in 19.28 seconds (files took 0.13447 seconds to load)
21 examples, 1 failure

Failed examples:

rspec ./spec/examples_spec.rb:157 # Processing example outputdirectory.tex should produce outputdirectory.pdf

pineapplemachine avatar Feb 24 '23 12:02 pineapplemachine

I will have a look, but before that: did you include the examples/outputdirectory.tex in your commits?

wtsnjp avatar Feb 24 '23 12:02 wtsnjp

...Well, now I did. Sorry about that

pineapplemachine avatar Feb 24 '23 12:02 pineapplemachine

The create_directory directive works on my local.

cf. https://relishapp.com/cucumber/aruba/v/0-11-0/docs/filesystem/create-directory

wtsnjp avatar Feb 24 '23 12:02 wtsnjp

:+1: Thank you for that resource, I was able to fix the output directory test so that it passes now.

Edit: Ha oh, and after doing that I saw that you had done the same fix in your commit. Anyway, it works now!

pineapplemachine avatar Feb 24 '23 16:02 pineapplemachine

Ok, I made the changes you requested

pineapplemachine avatar Feb 27 '23 12:02 pineapplemachine

Thank you very much. I think the implementation is much improved. A few more details need to be corrected.

wtsnjp avatar Mar 02 '23 12:03 wtsnjp

So where the makeglossaries command may look like this: makeglossaries -d my-output-dir my-doc.glo, the actual target path will be my-output-dir/my-doc.glo. If llmk does not know to check this actual target path, then it will look for the same target path given to makeglossaries, my-doc.glo without the preceding directory, and since no such *.glo file exists it will not proceed with the command. The purpose of target_path is to solve this problem.

I still don't understand why target_path is needed to be added. Can't we just set target to "my-output-dir/my-doc.glo" and give "%b.glo" in the args array? So the default programs.makeglossaries will look like this:

[programs.makeglossaries]
command = "makeglossaries"
target = "%o/%b.glo"
generated_target = true
postprocess = "latex"
opts = ['-d "%o"']
args = ["%b.glo"]

wtsnjp avatar Mar 02 '23 12:03 wtsnjp

I will make these changes.

As for the final comment, I'm sorry to say the reasoning is not as fresh in my mind as when I was responding to the question before. It seems like what you suggested would work? I looked at the code in detail again to refresh my memory. I think I may have been thrown off before by the construct_cmd function, which expects a target argument, even though the argument is unused. (The fn argument as well.) Possibly I assumed before that it was necessary to supply the target here, and that was why I did all of that to have both a target and a target_path, when it seems like it must have indeed been as simple as overriding the default program args for makeglossaries. I may see if I can revert the target_path change and do as you suggested, and see if it works, or what happens.

pineapplemachine avatar Mar 02 '23 22:03 pineapplemachine

Ok, this should be in a better state now. I made the changes you requested, and I reverted the target_path addition. It seems to be working alright.

pineapplemachine avatar Mar 03 '23 13:03 pineapplemachine

Thanks a lot for your great efforts. This PR looks excellent now, and I have merged it. Thanks also for fixing some originally existing typos in error messages and unnecessary parameters for a function.

wtsnjp avatar Mar 04 '23 11:03 wtsnjp