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

[collab] traceability poc/integration #78380

Merged

Conversation

nashif
Copy link
Member

@nashif nashif commented Sep 13, 2024

Note: This is a collab branch change and will be work in progress for a while before it is submitted for final review into main. So unless you are involved in the collab branch work or interested in this effort in particular, probably you do not want to spend much time on this now.

poc for integration of reqmgmt into documentation and tracing tests to requirements using doxygen and mlx traceability plugin.

not much to see here for now...

@zephyrbot
Copy link
Collaborator

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
reqmgmt N/A zephyrproject-rtos/reqmgmt@1f96a9f (main) N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-reqmgmt DNM This PR should not be merged (Do Not Merge) labels Sep 13, 2024
@nashif nashif changed the base branch from main to collab-safety September 13, 2024 09:17
@nashif nashif force-pushed the topic/req/traceability branch 3 times, most recently from 329dd5e to dfa03e5 Compare September 13, 2024 11:07
@nashif nashif requested a review from simhein September 13, 2024 12:28
Comment on lines -144 to -161
# API documentation coverage
python3 -m coverxygen --xml-dir doc/_build/html/doxygen/xml/ --src-dir include/ --output doc-coverage.info
# deprecated page causing issues
lcov --remove doc-coverage.info \*/deprecated > new.info
genhtml --no-function-coverage --no-branch-coverage new.info -o coverage-report

- name: compress-docs
run: |
tar --use-compress-program="xz -T0" -cf html-output.tar.xz --directory=doc/_build html
tar --use-compress-program="xz -T0" -cf api-output.tar.xz --directory=doc/_build html/doxygen/html
tar --use-compress-program="xz -T0" -cf api-coverage.tar.xz coverage-report

- name: upload-build
uses: actions/upload-artifact@v4
with:
name: html-output
path: html-output.tar.xz

- name: upload-api-coverage
uses: actions/upload-artifact@v4
with:
name: api-coverage
path: api-coverage.tar.xz

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason why the api coverage was removed? Or was it to keep the overhead minimal for the first draft of this PR?

Copy link
Member Author

@nashif nashif Sep 17, 2024

Choose a reason for hiding this comment

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

just temporary to remove noise

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really a problem to keep it though? Not sure it actually generates "noise", does it?

parse_nodes(d['NODES'], grouped)
return grouped

def parse_dng_csv(filename):
Copy link
Collaborator

Choose a reason for hiding this comment

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

That import wouldn't work with any of the strict doc exports right now. Is this intended to be left there to be extended in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove

@@ -1179,7 +1184,7 @@ SOURCE_BROWSER = NO
# documentation.
# The default value is: NO.

INLINE_SOURCES = NO
INLINE_SOURCES = YES
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this was activated to include the test function bodies into the doxygen documentation am I right?

Copy link
Member Author

Choose a reason for hiding this comment

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

dont remember tbh, I assume yes.

Comment on lines 87 to 90
@defgroup kernel_memprotect_tests Memory Protection
@ingroup all_tests
@{
@}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why group is defined in the group.dox and not like the others in the source file?

Copy link
Member Author

Choose a reason for hiding this comment

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

those exist in multiple places and tests, so putting this is a neutral place here.

Comment on lines +980 to +990
@ZEPHYR_BASE@/subsys/testsuite/ztest/include/ \
@ZEPHYR_BASE@/tests/kernel/semaphore/ \
@ZEPHYR_BASE@/tests/kernel/threads/ \
@ZEPHYR_BASE@/tests/kernel/mutex/ \
@ZEPHYR_BASE@/kernel/sem.c \
@ZEPHYR_BASE@/kernel/thread.c \
@ZEPHYR_BASE@/kernel/sched.c \
@ZEPHYR_BASE@/doc/requirements/requirements.dox
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the sources manually in this case is for testing purpose and the PoC?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is to get traceability to implementation.

Comment on lines 202 to 203
* Sets the IRQ line @p irq to trigger an interrupt based on the level or the
* edge of the signal. Valid values for @p trigger are _ARC_V2_INT_LEVEL and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we bring back these directly to the main branch instead of the collab branch if these tags are wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

they are already, there is a PR #78387

@simhein
Copy link
Collaborator

simhein commented Sep 17, 2024

Looks good to me for the poc integration

@simhein
Copy link
Collaborator

simhein commented Sep 24, 2024

I think if we can get rid of the compliance checks errors this batch could be merged to the branch.

An example for creating traceability between tests and requirements.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif
Copy link
Member Author

nashif commented Sep 26, 2024

I think if we can get rid of the compliance checks errors this batch could be merged to the branch.

done

Comment on lines +4364 to +4365
maintainers:
- nashif
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add me as a maintainer as well if you want, or as collaborator.

Pull requirements in so we can convert them to dox and integrate with
doxygen.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Script to convert requirements to dox.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Integrate into doc workflow.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Sep 27, 2024
@nashif nashif removed the DNM This PR should not be merged (Do Not Merge) label Sep 27, 2024
@simhein
Copy link
Collaborator

simhein commented Oct 21, 2024

@nashif should we merge this to move forward with the collab branch? Also should we activate the api-coverage generation again to not forget about it in a later stage?

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

making my comments non-blocking -- this looks like this going in the right direction :)


- name: west setup
run: |
west init -l .

- name: prcoess requirements
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- name: prcoess requirements
- name: process requirements

Comment on lines -144 to -161
# API documentation coverage
python3 -m coverxygen --xml-dir doc/_build/html/doxygen/xml/ --src-dir include/ --output doc-coverage.info
# deprecated page causing issues
lcov --remove doc-coverage.info \*/deprecated > new.info
genhtml --no-function-coverage --no-branch-coverage new.info -o coverage-report

- name: compress-docs
run: |
tar --use-compress-program="xz -T0" -cf html-output.tar.xz --directory=doc/_build html
tar --use-compress-program="xz -T0" -cf api-output.tar.xz --directory=doc/_build html/doxygen/html
tar --use-compress-program="xz -T0" -cf api-coverage.tar.xz coverage-report

- name: upload-build
uses: actions/upload-artifact@v4
with:
name: html-output
path: html-output.tar.xz

- name: upload-api-coverage
uses: actions/upload-artifact@v4
with:
name: api-coverage
path: api-coverage.tar.xz

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really a problem to keep it though? Not sure it actually generates "noise", does it?

Comment on lines +983 to +990
@ZEPHYR_BASE@/subsys/testsuite/ztest/include/ \
@ZEPHYR_BASE@/tests/kernel/semaphore/ \
@ZEPHYR_BASE@/tests/kernel/threads/ \
@ZEPHYR_BASE@/tests/kernel/mutex/ \
@ZEPHYR_BASE@/kernel/sem.c \
@ZEPHYR_BASE@/kernel/thread.c \
@ZEPHYR_BASE@/kernel/sched.c \
@ZEPHYR_BASE@/doc/requirements/requirements.dox
Copy link
Collaborator

Choose a reason for hiding this comment

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

mixed tabs/spaces

for r in grouped.keys():
comp = grouped[r]
counter += 1
req.write(f"\n@section REQSEC{counter} {r}\n\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggest to use a different approach to generate the REQSECXXX name, as you probably don't want it to change unnecessarily. I.e. right now http://localhost:8081/Requirements.html#REQSEC1 links to "Hardware Architecture Interface" for me, and I might end up sharing this link thinking it points to this particular section, but it can break at any time
While not perfect, probably better to use_lowercase_and_underscore_version_of_the_title as the section name?

@nashif
Copy link
Member Author

nashif commented Oct 21, 2024

Is it really a problem to keep it though? Not sure it actually generates "noise", does it?

the introduction of the new tags does intefere with the coverage, I need to look into it in details later and re-enable.

@nashif nashif merged commit 1396bc2 into zephyrproject-rtos:collab-safety Oct 21, 2024
26 of 27 checks passed
@nashif nashif deleted the topic/req/traceability branch October 21, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants