Skip to content

Refactor build/dependency documentation + new markdown parser (myst-parser) #1914

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

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

jgoeders
Copy link
Contributor

@jgoeders jgoeders commented Nov 17, 2021

Overview

This PR makes two changes at once (my apologies, it would be better if they were separate, but they become quite intertwined while getting this to all work):

  1. This PR provides a few updates to ease the environment setup for new users by refactoring the build documentation that was spread across multiple different pages.
  2. Migrates the Sphinx markdown processor from recommonmark (now deprecated) to myst-parser (the newly official markdown processor).

Refactoring Build Documentation

Multiple changes were made to the instructions on building and setting up VTR.

1.I've moved the required packages into a install_apt_packages.sh file. The list of packages has been changed somewhat, and I've removed old packages that we aren't using anymore. I've tested on a fresh Ubuntu 18 and 20 system.

  1. I've created a make evn target to setup a Python environment.

    With the move to using Python for the runner scripts, and the inclusion of a requirements.txt file of required Python packages, I think it would be nice if we provided an automated mechanism to create a Python virtual environment and install the necessary packages. To me this seems like good practice versus having users modify their system Python version, especially since our requirements.txt is requiring specific versions of certain packages.

  2. I've refactored the build documentation. We currently have build documentation in three places, and it's quite inconsistent about what information is where:

With these changes:

  • The quick start guide now includes information about setting up necessary dependencies
  • All required build information is moved to a single "Building VTR" page.
  • Optional build information (debug mode, sanitizers, other platforms, etc), is moved to a single "Optional Build Information" page.

Changing Markdown Processor

While refactoring this documentation I discovered issue #1941 where certain internal markdown links were broken. This seemed to be a byproduct of our efforts to fix links between the Sphinx documentation and our top-level Markdown files (ie CONTRIBUTING.md).

Rather than trying to fix our broken workaround, which was still using recommonmark (deprecated Markdown processor), I migrated our Sphinx build to use the newer (and official) myst-parser. At first it wasn't obvious how to support links between our top-level Markdown files and our Sphinx docs, but I managed to develop a solution.

This involved replacing symlinks that we had from our docs/src to the top-level Markdown files, and instead use some stub files with myst-parser's relative-docs feature. Example stub file:

```{include} ../../BUILDING.md
:relative-docs: doc/src
```

This approach seems to fix all of our linking issues. I've also updated several broken links as I've cleaned things up.

Related Issue

#1941

How Has This Been Tested?

I've compared the Sphinx build log outputs before and after this change to ensure no new errors/broken links were created.

Types of changes

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@github-actions github-actions bot added build Build system lang-make CMake/Make code labels Nov 17, 2021
@github-actions github-actions bot added lang-shell Shell scripts (bash etc.) scripts Utility & Infrastructure scripts labels Dec 3, 2021
@jgoeders jgoeders changed the title python virtual environment Update environment setup instructions (+python virtual environment) Dec 3, 2021
@jgoeders jgoeders force-pushed the venv branch 2 times, most recently from 4d4a066 to 9c34bfd Compare December 3, 2021 20:16
@github-actions github-actions bot added the docs Documentation label Dec 3, 2021
@jgoeders jgoeders marked this pull request as draft December 17, 2021 02:18
@jgoeders
Copy link
Contributor Author

jgoeders commented Jan 6, 2022

This fixes #1941

@jgoeders jgoeders linked an issue Jan 6, 2022 that may be closed by this pull request
@jgoeders jgoeders changed the title Update environment setup instructions (+python virtual environment) Refactor build/dependency documentation + new markdown parser (myst-parser) Jan 6, 2022
@jgoeders jgoeders marked this pull request as ready for review January 6, 2022 20:17
@jgoeders jgoeders linked an issue Jan 7, 2022 that may be closed by this pull request
@jgoeders
Copy link
Contributor Author

jgoeders commented Jan 7, 2022

@vaughnbetz @acomodi

This is ready for review.

## Unix-like ##
For unix-like systems we provide a wrapper Makefile which supports the traditional `make` and `make clean` commands, but calls CMake behind the scenes.
make env
source .venv/bin/activate
Copy link
Contributor

@vaughnbetz vaughnbetz Jan 7, 2022

Choose a reason for hiding this comment

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

Can / should the source .venv/bin/activate command be done by make env? (one less command to run).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately there's no way to affect the environment of the caller's process (the user's terminal) from within a Makefile command. https://stackoverflow.com/a/7508273

What I've seen done in other projects (including several Symbiflow projects) is to activate the environment from within a Makefile target, which would have effect for only that target. Since we don't run the VTR flow using Makefile targets (but rather have users directly invoke the Python scripts) that doesn't really help.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's a good reason :).

BUILDING.md Outdated
* Xft (libXft + libX11)
* fontconfig
* libgtk-3-dev
**Note:** If you chose to install the Python virtual environment, you will need to remember to activate it on any new terminal window you use, before you can run the VTR flow or regressions tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to give the exact command to run to activate the environment here, maybe in brackets. Especially useful if the activate gets moved into the make env command, so you only have to run this command in other windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


> make env
> source .venv/bin/activate
> pip install -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Should make env run the next two commands too (fewer commands to run)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or are you trying to show them how to do this system wide or in a virtual envronment? If so, suggest putting a command on the two virtual environment lines.

make env # optional: install python virtual environment
source .venv/bin/activate # optional: activate python virtual environment
pip install -r requirements.txt # install python packages (in virtual environment if prior commands run, system wide otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't collapse commands as noted above, but I did add the comments. Good idea.

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

Looks great; just a few comments.


> make env
> source .venv/bin/activate
> pip install -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Or are you trying to show them how to do this system wide or in a virtual envronment? If so, suggest putting a command on the two virtual environment lines.

make env # optional: install python virtual environment
source .venv/bin/activate # optional: activate python virtual environment
pip install -r requirements.txt # install python packages (in virtual environment if prior commands run, system wide otherwise)

Copy link
Collaborator

@acomodi acomodi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

LGTM

@vaughnbetz vaughnbetz merged commit 116f30c into verilog-to-routing:master Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system docs Documentation lang-make CMake/Make code lang-shell Shell scripts (bash etc.) scripts Utility & Infrastructure scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recommonmark internal links + move to myst-parser? No Module name 'prettytable'
3 participants