Skip to content

Conversation

@fingolfin
Copy link
Member

No description provided.

@fingolfin
Copy link
Member Author

However this script does not actually work for me, does it for anyone?

For one the call to find seems to miss its first argument referencing the path from which to start the search (e.g. .). And then latex does not find TikZHeader.tex presumably because it is run from the wrong directory?

Maybe @schnellecom knows as he added this script in the first place

@codecov
Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (132bf7e) to head (ab34db6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   70.13%   70.13%           
=======================================
  Files          62       62           
  Lines       16936    16936           
=======================================
  Hits        11878    11878           
  Misses       5058     5058           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MeikeWeiss MeikeWeiss requested a review from schnellecom May 16, 2025 08:58
@schnellecom
Copy link
Collaborator

In which directory do you execute the script? I tried on debian in the tikz-files directory and the script seems to do what is expected (it runs for like 2h, so I cancelled it before). I will talk to some other people with different OSes and check if I can reproduce the error.

@schnellecom
Copy link
Collaborator

Check on another Linux based system where it seems to work. However on another Mx MacBook there also seems to be an error, see here:
image
Is that the same error you are having @fingolfin ?

@schnellecom
Copy link
Collaborator

As to the intended functionality:
the for loop is supposed to find all .tex files that start with _Wrapper_* and then run pdflatex and htlatex on them to generate the .svg and .pdf versions. The .svg is then supposed to be moved to the ../images/ folder.

Checking only for .tex files starting with _Wrapper_ originated by the original code (which generated all images before generating the manual) using a separate Wrapper file which contained the header and footer of the latex. We combined them directly into single files, so it is easier as a user to see what is happening and change it if desired.

@fingolfin
Copy link
Member Author

Part of the problem is that this is all the instructions say:

To compile the pictures, you can run the following script:
```
doc/tikz-files/recompile-images.sh
```    

Since the script is given with a full path I assumed it had to be run from the package root directory.

I've updated this PR with a proposal for making this perhaps a bit clearer.

But this does not e.g. specify in which directory to run this.

The invocation of find is not POSIX compliant and hence fails on macOS (and I am surprised that it doesn't on Linux but apparently this is a GNU extension): it is missing the initial path, the "starting point" for the search. This is fixed by my second commit in this PR.

@fingolfin
Copy link
Member Author

BTW you could make this a lot faster by using GNU parallel then at least multiple cores could work on it at once. Alternatively, just turn the script into a simple Makefile then you can do e.g. make -j 8 to compile 8 documents at once. I'd be happy to provide such a Makefile for experiments if you are interested.

It also seems this modifies PDFs which you stored in the git repo. This is because each time you regenerate the PDF it changes slightly, due to the included timestamps.

To avoid this you could add export SOURCE_DATE_EPOCH=1 at the start of the script, then things should become idempotent.

Finally, might also wish to consider using lualatex instead of pdflatex: it will be slower, but it also produces smaller PDFs (e.g. _Wrapper_properties-1.pdf goes from 12299 to 4672 bytes).

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

Successfully merging this pull request may close these issues.

3 participants