-
Notifications
You must be signed in to change notification settings - Fork 416
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
Conversation
4d4a066
to
9c34bfd
Compare
This fixes #1941 |
Signed-off-by: Jeff Goeders <[email protected]>
Signed-off-by: Jeff Goeders <[email protected]>
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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/src/quickstart/index.rst
Outdated
|
||
> make env | ||
> source .venv/bin/activate | ||
> pip install -r requirements.txt |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
doc/src/quickstart/index.rst
Outdated
|
||
> make env | ||
> source .venv/bin/activate | ||
> pip install -r requirements.txt |
There was a problem hiding this comment.
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)
Signed-off-by: Jeff Goeders <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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):
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.
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.
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:
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: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
Checklist: