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.

Sign up for GitHub

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

Jump to bottom

Doc for adding a new part to PrjXray. #1486

Merged
merged 4 commits into from
Nov 20, 2020
Merged

Doc for adding a new part to PrjXray. #1486

merged 4 commits into from
Nov 20, 2020

Conversation

tcal-x
Copy link
Contributor

@tcal-x tcal-x commented Nov 6, 2020

Signed-off-by: Tim Callahan tcal@google.com

@tcal-x tcal-x requested review from mithro and acomodi November 6, 2020 21:52
@mithro mithro requested review from litghost and mkurc-ant and removed request for mithro November 6, 2020 22:47
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated
@@ -0,0 +1,203 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be in docs/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense. The question is, /only/ in docs as .rst, or also have an .md version? I noticed that the getting started readme exists in both formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ask for re-reviews after updating the content here, and then when it looks good, I'll convert it to an .rst file under docs/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkurc-ant , @litghost , the new doc (docs/newpart.rst) is on readthedocs, ready for review. Ignore the NEWPART.md file; I'll delete it before merging. Start here: https://symbiflow--1486.org.readthedocs.build/projects/prjxray/en/1486/db_dev_process/index.html (to see where I've put the new page in the hierarchy), then click on "Guide to adding...".

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it time to delete this file?

litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated

* Rerun the top make command, `make -j32 MAX_VIVADO_PROCESS=32 db-part-only-artix7_100t`

### Step 5 ###
Copy link
Contributor

Choose a reason for hiding this comment

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

When this is committed, add a comment to #1476 to remove this step once we have updated the part processing stuff. The instructions here also need to be updated as part of #1475 too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. I'll leave this comment unresolved, and add a comment to those PRs when this doc's final location is determined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also need to check if f4pga/f4pga-arch-defs#1791 affects my instructions (under "Database Updates")

litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 6, 2020
View reviewed changes
NEWPART.md Outdated Show resolved Hide resolved
mkurc-ant
mkurc-ant reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@mkurc-ant mkurc-ant left a comment

Choose a reason for hiding this comment

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

You have written this doc more like a report of what you did and how. In my opinion an instruction should be written in a more impersonal way.

NEWPART.md Outdated Show resolved Hide resolved
NEWPART.md Outdated Show resolved Hide resolved
NEWPART.md Outdated
Clone your fork (replace "tcal-x" with your GitHub ID):

```
git clone git@github.com:tcal-x/prjxray.git
Copy link
Contributor

Choose a reason for hiding this comment

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

You may just point to the SymbiFlow/prjxray. There will be no need for instructing a user to replace your github id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced "tcal-x" with "", since I don't want them to start hacking directly on their clone of SymbiFlow's repo. Yes, if they know enough git they will know how to update things so they can commit to their own fork later, but it seems simpler if they start with their own fork...

@litghost litghost mentioned this pull request Nov 17, 2020
Closed
@tcal-x tcal-x changed the title Doc for adding a new part to PrjXray. WIP DNM Doc for adding a new part to PrjXray. Nov 18, 2020
@tcal-x tcal-x marked this pull request as draft November 18, 2020 22:45
@tcal-x tcal-x changed the title WIP DNM Doc for adding a new part to PrjXray. Doc for adding a new part to PrjXray. Nov 20, 2020
@tcal-x tcal-x marked this pull request as ready for review November 20, 2020 16:42
litghost
litghost reviewed Nov 20, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
litghost
litghost reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Overall looks great! Let's cleanup the handful of comments, and get this merged!

@tcal-x
Doc for adding a new part to PrjXray.
94f14ce 
Signed-off-by: Tim Callahan <tcal@google.com>
@tcal-x
rm NEWPART.md; update README pointer; small edits.
7f77e9f 
Signed-off-by: Tim Callahan <tcal@google.com>
litghost
litghost approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

LGTM

@tcal-x
Some final edits (fix section level of "Database Updates").
c502aa0 
Signed-off-by: Tim Callahan <tcal@google.com>
@tcal-x
Fix trailing space issues.
d2de96b 
Signed-off-by: Tim Callahan <tcal@google.com>
@tcal-x tcal-x merged commit 2353b22 into f4pga:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@litghost litghost litghost approved these changes

@acomodi acomodi Awaiting requested review from acomodi

@mkurc-ant mkurc-ant Awaiting requested review from mkurc-ant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tcal-x @litghost @mkurc-ant