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

tests: run html and latexpdf builders for all tests #91

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

oharboe
Copy link
Contributor

@oharboe oharboe commented Sep 21, 2023

These two builders are substantially different, so worth running both.

Other builders, such as DirectoryHTMLBuilder, doesn't result in different codepaths for wavedrom graphics

@oharboe oharboe force-pushed the latexpdf-tests branch 2 times, most recently from cbfe1f4 to 1d17156 Compare September 21, 2023 04:55
@oharboe
Copy link
Contributor Author

oharboe commented Sep 21, 2023

graphicsmagick-imagemagick-compat might be a better dependency than inkscape, but let's see if this passes tests now that I added inkscape as a dependency first.

$ apt-file search /usr/bin/convert | grep -w convert
graphicsmagick-imagemagick-compat: /usr/bin/convert

@mithro
Copy link
Member

mithro commented Sep 21, 2023

Doesn't appear that inkscape is available via direct conda channels, might need to add conda-forge as a channel?

@oharboe
Copy link
Contributor Author

oharboe commented Sep 21, 2023

graphicsmagick-imagemagick-compat

How about switching to "convert" from the Ubuntu graphicsmagick-imagemagick-compat package?

sphinx.ext.imgconverter relies on "convert", so perhaps a more suitable dependency?

If "convert" exists, I can make change from inkscape to convert...

@oharboe oharboe force-pushed the latexpdf-tests branch 2 times, most recently from 03f376e to b9239ab Compare September 21, 2023 05:44
@mithro
Copy link
Member

mithro commented Sep 21, 2023

If convert works for svg files, then go ahead.

@mithro
Copy link
Member

mithro commented Sep 21, 2023

The CI looks like it is failing because no pdf file was created?

@oharboe
Copy link
Contributor Author

oharboe commented Sep 21, 2023

The CI looks like it is failing because no pdf file was created?

Using "convert" comes with it's own problems... Some sort of security vulnerability in parsing .svg files from the web...

$ convert ./docs/fpga-microarchitecture/structural-schematics/ce.svg foo.pdf
convert-im6.q16: attempt to perform an operation not allowed by the security policy `PDF' @ error/constitute.c/IsCoderAuthorized/426.

@oharboe
Copy link
Contributor Author

oharboe commented Sep 21, 2023

@mithro The failure is actually in yosys, which is odd... see assert in line 300. This PR doesn't modify that part of the code. Strange...

yowasp_yosys.run_yosys() returns an error code that is not checked....

https://github.com/YoWASP/yosys/blob/3767b1d481cd1197211dbdc6e509c4748104dba0/pypi/yowasp_yosys/__init__.py#L11C27-L11C35

https://github.com/YoWASP/runtime/blob/2d89db49c29a382a6801a45b09e36516c71d32b3/yowasp_runtime/__init__.py#L18

writing... Running YoWASP yosys: ['-q', '-p', 'prep -top ADDER ; cd ADDER; script /home/runner/work/sphinxcontrib-hdl-diagrams/sphinxcontrib-hdl-diagrams/tests/build/TestYosysScript/test_yosys_script/yosys_script.ys; show -format pdf -prefix /home/runner/work/sphinxcontrib-hdl-diagrams/sphinxcontrib-hdl-diagrams/tests/build/TestYosysScript/test_yosys_script/build/test_yosys_script-35-build-TestYosysScript-test_yosys_script-adder', '/home/runner/work/sphinxcontrib-hdl-diagrams/sphinxcontrib-hdl-diagrams/tests/build/TestYosysScript/test_yosys_script/adder.v']
failed

image

@oharboe oharboe force-pushed the latexpdf-tests branch 2 times, most recently from de7e755 to 069485f Compare September 22, 2023 05:43
Run html and latexpdf builders for all tests

These two builders are substantially different, so worth
running both.

Other builders, such as DirectoryHTMLBuilder, doesn't result
in different codepaths for wavedrom graphics

imagemagick has a default security policy that disables
svg to pdf conversion as this conversion goes through
ghostscript that uses a turing complete .eps language.

Signed-off-by: Øyvind Harboe <[email protected]>
@oharboe
Copy link
Contributor Author

oharboe commented Sep 22, 2023

@mithro Tested locally, but it seems like it runs out of memory... I am not surprised latex needs substantially more memory than the html output...

image

@mithro
Copy link
Member

mithro commented Sep 22, 2023

Overall, this change looks fine.

From what I can see, this doesn't change the memory usage?

I tried adding the following;

diff --git a/conda.mk b/conda.mk
index cf35d8b..6c60817 100644
--- a/conda.mk
+++ b/conda.mk
@@ -142,7 +142,7 @@ $(CONDA_ENVS_DIR): $(CONDA_PYTHON)
        $(MKDIR) "$(CONDA_ENVS_DIR)"
 
 $(CONDA_ENV_PYTHON): $(ENVIRONMENT_FILE) $(REQUIREMENTS_FILE) | $(CONDA_PYTHON) $(CONDA_PKGS_DEP) $(CONDA_ENVS_DIR) $(CONDA_PYVENV)
-       $(IN_CONDA_ENV_BASE) conda env update --name $(CONDA_ENV_NAME) --file $(ENVIRONMENT_FILE)
+       $(IN_CONDA_ENV_BASE) /usr/bin/time --verbose conda env update --name $(CONDA_ENV_NAME) --file $(ENVIRONMENT_FILE)
        $(TOUCH) "$(CONDA_ENV_PYTHON)"
 
 env:: $(CONDA_ENV_PYTHON)

Before - Maximum resident set size (kbytes): 3493760
After - Maximum resident set size (kbytes): 3305444

@oharboe
Copy link
Contributor Author

oharboe commented Sep 22, 2023

@mithro The extra memory consumption is not surprising: I think it comes from testing latex in addition to html...

@oharboe
Copy link
Contributor Author

oharboe commented Sep 22, 2023

@mithro I cant test anything more until Monday... I am unsure on what to try next...

@mithro
Copy link
Member

mithro commented Sep 22, 2023

From my reading of the output, the command seems to be failing during the conda environment setup before anything like tests are run.

@mithro
Copy link
Member

mithro commented Sep 22, 2023

I'm going to proceed with merging.

@mithro mithro merged commit 2ef1795 into SymbiFlow:master Sep 22, 2023
4 of 5 checks passed
@oharboe
Copy link
Contributor Author

oharboe commented Sep 23, 2023

I'm going to proceed with merging.

Thanks!

It would be great to have the regression test working.

I think rsvg-r is the least problematic depdendency. inkscape and convert are tools on top of a raft of libraries and rsvg-r seems like a small leaf dependency.

@oharboe oharboe deleted the latexpdf-tests branch September 23, 2023 07:20
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.

2 participants