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

Test flatten option #53

Merged
merged 2 commits into from
Sep 17, 2020
Merged

Test flatten option #53

merged 2 commits into from
Sep 17, 2020

Conversation

rw1nkler
Copy link
Contributor

@rw1nkler rw1nkler commented Aug 28, 2020

This PR adds the basic tests for the flatten option in the verilog-diagram directive. The test consists of the Sphinx run in which the module black boxes should be resolved (with the flatten option) for all types of diagrams. The example uses the full adder circuit, which consists of two half adder modules.

In order to get the possibility of checking the output HTMLs, I created the next Travis CI job. The job generates the HTMLs from all test runs, converts them to the single HTML file using monolith and adds them to the GitHub Pages. The Travis setup requires GitHub token with the public_repo access rights. The token should be added as a Travis environment variable with the GITHUB_PAGES_TOKEN name.

Preview of the GitHub pages setup can be found here:
https://rw1nkler.github.io/sphinxcontrib-verilog-diagrams/

The GitHub Pages functionality has been moved to #58

Here is the flatten test preview:
https://rw1nkler.github.io/sphinxcontrib-verilog-diagrams/test_flatten.html

test_flatten

Resolves: #3


#52 should be merged first

@mithro
Copy link
Member

mithro commented Aug 29, 2020

Rather than using travis, could we just use readthedocs here?

@rw1nkler
Copy link
Contributor Author

rw1nkler commented Aug 31, 2020

Rather than using travis, could we just use readthedocs here?

I wanted to run the same file with multiple sphinx configurations, which is hard to achieve on readthedocs. In example, this would allow testing the extension with diagrams format (global setting) set once to png and in the second time to svg. The GitHub pages have the disadvantage that the result will be visible only after merging the changes, which is not that useful for testing. But Travis can be integrated with Google Storage for storing artefacts (here the test HTMLs). Maybe this would be the most flexible approach.

If this does not convince you, we can limit ourselves to one Sphinx configuration and create the second RTD project for tests. This should work fine too and should give the direct preview in the CI.

This PR should be merged after the #52. The previous PR is much less controversial.

@rw1nkler rw1nkler changed the title Test flatten option [WIP] Test flatten option Aug 31, 2020
@mithro
Copy link
Member

mithro commented Sep 1, 2020

Thanks for the explanation!

You can set up github actions which allow for "test deployments" of github pages stuff. I have never done that however...

@rw1nkler
Copy link
Contributor Author

rw1nkler commented Sep 4, 2020

You can set up github actions which allow for "test deployments" of github pages stuff.

I've checked the GitHub Actions available in the GitHub market, but it seems that none of them offers the test deployment functionality. This might be related to the GitHub pages limitation - it only allows for publishing one version of the website.

@rw1nkler
Copy link
Contributor Author

I believe that we should create a Kokoro setup or upload the artifacts to i.e.symbiflow-scratch bucket with Travis CI.
@mithro What do you think about this problem?

Copy link
Member

@mithro mithro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pushing to github pages is fine. I'm pretty sure we don't need to use monolith though?

@mithro
Copy link
Member

mithro commented Sep 14, 2020

@rw1nkler Can you also split the CI change from the other changes?

@rw1nkler rw1nkler marked this pull request as ready for review September 15, 2020 14:29
@rw1nkler rw1nkler changed the title [WIP] Test flatten option Test flatten option Sep 15, 2020
@rw1nkler
Copy link
Contributor Author

@rw1nkler Can you also split the CI change from the other changes?

I have created a second PR with the GitHub pages deployment (PR #58)

@mithro
Copy link
Member

mithro commented Sep 16, 2020

This is going to need a rebase now.

@rw1nkler rw1nkler force-pushed the test_flatten branch 2 times, most recently from b1f5dd7 to c84056d Compare September 17, 2020 13:39
This commit adds a basic test for the flatten option in the
verilog-diagram directive.

Signed-off-by: Robert Winkler <[email protected]>
This commit changes the HTML tag which is used for SVG images
from <object> to <img>. This fixes scalability of images on websites.

Signed-off-by: Robert Winkler <[email protected]>
@rw1nkler
Copy link
Contributor Author

@mithro I rebased the PR

@mithro mithro merged commit 1ee9612 into SymbiFlow:master Sep 17, 2020
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.

Add better tests for flatten / non-flatten flow
2 participants