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

[ipgen,tpl,cmdgen] Remove earlgrey CMDGEN output from ipgen templates #24973

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

matutem
Copy link
Contributor

@matutem matutem commented Oct 31, 2024

I verified the resulting generated md files are unchanged. This requires running make -C hw, followed by cmdgen.py for the affected generated files.

@matutem matutem requested a review from andreaskurth October 31, 2024 22:32
@matutem matutem changed the title [doc,cmdgen] Remove useless text bracketed by BEGIN END CMDGEN [doc,cmdgen] Remove text bracketed by BEGIN and END CMDGEN Oct 31, 2024
@andreaskurth
Copy link
Contributor

Thanks @matutem. I agree with the intention of this PR, but it doesn't seem to be aligned with what happens when make -C hw is run: In hw/Makefile (for the top target), topgen is run but cmdgen isn't.

Unfortunately it's not as simple as adding ${PRJ_DIR}/util/cmdgen.py -u hw/$*/**/*.md there because the working directory then is wrong

@andreaskurth
Copy link
Contributor

andreaskurth commented Nov 1, 2024

Unfortunately it's not as simple as adding ${PRJ_DIR}/util/cmdgen.py -u hw/$*/**/*.md there because the working directory then is wrong

PR #24986 #24989 should solve this problem.

Once that PR is merged, we can make the following change

diff --git a/hw/Makefile b/hw/Makefile
index 238e348a7a..1a5c2bcc31 100644
--- a/hw/Makefile
+++ b/hw/Makefile
@@ -136,6 +136,7 @@ topgen_rust_pkg: topgen_rust
 top: $(tops_gen) $(tops_reg)
 $(tops_gen): %_gen:
        ${PRJ_DIR}/util/topgen.py -t $(top-hjson) -o ${PRJ_DIR}/hw/$*/ ${toolflags}
+       ${PRJ_DIR}/util/cmdgen.py -u hw/$*/**/*.md
 
 $(tops_reg): %_reg:
        mkdir -p ${REG_OUTPUT_DV_DIR}/$*

either as a new commit at the beginning of this PR or in a separate PR.

Then this PR should be ready.

@matutem
Copy link
Contributor Author

matutem commented Nov 1, 2024

I will file an issue and a separate PR for adding cmdgen in the Makefile target for top. I think it deserves better documentation since otherwise check-generated would fail. It is weird we didn't catch it for so long: a quick grep suggests we always have the top earlgrey text between brackets, which would break for multi-top, and perhaps we never checked this for englishbreakfast? I'll loop you in for sure.

@matutem
Copy link
Contributor Author

matutem commented Nov 1, 2024

Filed #24991.

@andreaskurth
Copy link
Contributor

Force-pushed to rebase on master including PR #24993

Copy link
Contributor

@hcallahan-lowrisc hcallahan-lowrisc left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for fixing this properly for future multi-top.

@hcallahan-lowrisc hcallahan-lowrisc changed the title [doc,cmdgen] Remove text bracketed by BEGIN and END CMDGEN [ipgen,tpl,cmdgen] Remove earlgrey CMDGEN output from ipgen templates Nov 4, 2024
I verified the resulting generated md files are unchanged.
This requires running make -C hw, followed by cmdgen.py for
the affected generated files.

Signed-off-by: Guillermo Maturana <[email protected]>
@matutem matutem merged commit 86de37b into lowRISC:master Nov 5, 2024
41 checks passed
@matutem matutem deleted the doc_cmdgen branch November 5, 2024 00:11
@Razer6
Copy link
Member

Razer6 commented Nov 5, 2024

With this change, make -k -C hw top always deletes all documentation in the output such as: hw/top_earlgrey/ip_autogen/<IP>/doc/(interfaces|registers).md

@pamaury
Copy link
Contributor

pamaury commented Nov 7, 2024

Since this change, running topgen or make now seems to delete the top_earlgrey documentation which is problematic. Is that an unintentional consequence? Are we supposed to run another make command to restore the documentation?

@hcallahan-lowrisc
Copy link
Contributor

hcallahan-lowrisc commented Nov 11, 2024

With this change, you would have to run make -C hw top_and_cmdgen to get back to the same clean repository state with cmdgen-rendered earlgrey documentation.
I think this API change was unintentional, I'll create a PR to change it back so the 'top' target gets us back to the clean state (unless anyone wants to advocate for some different API here for this development tool)

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.

5 participants