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

docs: Add Makefile generation target and CI testing #124

Merged
merged 2 commits into from
Nov 21, 2023

Conversation

Zildj1an
Copy link
Contributor

Add Makefile target to generate cargo documentation and update README accordingly.

Makefile Outdated Show resolved Hide resolved
@00xc
Copy link
Member

00xc commented Oct 12, 2023

Would it make sense to add the documentation build process to Github CI? We can make the build fail on warnings with:

RUSTDOCFLAGS="-D warnings" cargo doc --all-features --document-private-items

@Zildj1an
Copy link
Contributor Author

Would it make sense to add the documentation build process to Github CI? We can make the build fail on warnings with:

RUSTDOCFLAGS="-D warnings" cargo doc --all-features --document-private-items

Yes, that would be very positive, agreed.

Makefile Outdated Show resolved Hide resolved
@Zildj1an Zildj1an changed the title docs: Add Makefile generation target docs: Add Makefile generation target and CI testing Oct 19, 2023
@Zildj1an
Copy link
Contributor Author

I have combined the PR #132 with this one and added --all-features. Thanks @00xc

@stefano-garzarella
Copy link
Member

IIUC from https://doc.rust-lang.org/cargo/commands/cargo-doc.html, we will not generate the svsm binary documentation, since we also have the svsm library:

The binary will be skipped if its name is the same as the lib target.

Indeed, using --bins will show only the binary docs.
I don't know if there is a solution to have both, but I think for now it is okay to generate only the library documentation.

00xc added a commit to 00xc/svsm that referenced this pull request Oct 20, 2023
Fix a broken link in the generated documentation for
`RawAllocMapping`, which will cause a CI break with the upcoming
changes in coconut-svsm#124.

Signed-off-by: Carlos López <[email protected]>
@00xc
Copy link
Member

00xc commented Oct 20, 2023

We still need to fail, at least in CI, if there are warnings in the build. This can be done in the Makefile:

diff --git a/Makefile b/Makefile
index 8725dcf..6beaf91 100644
--- a/Makefile
+++ b/Makefile
@@ -20,7 +20,7 @@ test:
        cargo test --target=x86_64-unknown-linux-gnu
 
 doc:
-       cargo doc --open --all-features --document-private-items
+       RUSTDOCFLAGS="-D warnings" cargo doc --open --all-features --document-private-items
 
 utils/gen_meta: utils/gen_meta.c
        cc -O3 -Wall -o $@ $<

Or in Github Actions:

diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index ca67d19..04121e9 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -57,3 +57,5 @@ jobs:
 
       - name: Check documentation
         run: make doc
+        env:
+          RUSTDOCFLAGS: -D warnings

Not sure which one is the best approach.

Add Makefile target to generate cargo documentation and update README
accordingly.

Signed-off-by: Carlos Bilbao <[email protected]>
Check documentation didn't break on pull requests.

Signed-off-by: Carlos Bilbao <[email protected]>
@Zildj1an
Copy link
Contributor Author

Zildj1an commented Oct 23, 2023

Thanks @00xc I have updated setting RUSTDOCFLAGS in the workflow and rebased. But we got:

error: unresolved link to `pages`
  --> src/mm/vm/mapping/rawalloc.rs:23:39
   |
23 |     /// Number of pages required in [`pages`]
   |                                       ^^^^^ no item named `pages` in scope
   |
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
   = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`

error: could not document `svsm`

which makes me think we actually don't want to fail on warnings, right?

@00xc
Copy link
Member

00xc commented Oct 27, 2023

Thanks @00xc I have updated setting RUSTDOCFLAGS in the workflow and rebased. But we got:
...
which makes me think we actually don't want to fail on warnings, right?

I would want them, it's just a matter of fixing remaining warnings before merging this.

00xc added a commit to 00xc/svsm that referenced this pull request Oct 27, 2023
Fix a broken link in the generated documentation for
`RawAllocMapping`, which will cause a CI break with the upcoming
changes in coconut-svsm#124.

Signed-off-by: Carlos López <[email protected]>
00xc added a commit to 00xc/svsm that referenced this pull request Oct 27, 2023
Fix a broken link in the generated documentation for
`RawAllocMapping`, which will cause a CI break with the upcoming
changes in coconut-svsm#124.

Signed-off-by: Carlos López <[email protected]>
Copy link
Contributor

@osteffenrh osteffenrh left a comment

Choose a reason for hiding this comment

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

Looks good.
Remember to merge #124 first :-)

@Zildj1an
Copy link
Contributor Author

Zildj1an commented Oct 31, 2023

Thanks @00xc I have updated setting RUSTDOCFLAGS in the workflow and rebased. But we got:
...
which makes me think we actually don't want to fail on warnings, right?

I would want them, it's just a matter of fixing remaining warnings before merging this.

@00xc but I don't think that this warning is generated by this PR?

error: unresolved link to `pages`
  --> src/mm/vm/mapping/rawalloc.rs:23:39
   |
23 |     /// Number of pages required in [`pages`]
   |                                       ^^^^^ no item named `pages` in scope
   |
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
   = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`

error: could not document `svsm`

@00xc
Copy link
Member

00xc commented Oct 31, 2023

@00xc but I don't think that this warning is generated by this PR?

error: unresolved link to `pages`
  --> src/mm/vm/mapping/rawalloc.rs:23:39
   |
23 |     /// Number of pages required in [`pages`]
   |                                       ^^^^^ no item named `pages` in scope
   |
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
   = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`

error: could not document `svsm`

No, it was introduced before. It should be fixed by #144.

Copy link
Member

@00xc 00xc left a comment

Choose a reason for hiding this comment

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

LGTM. Once the PR mentioned above is merged CI should no longer fail.

@joergroedel joergroedel merged commit f100d9e into coconut-svsm:main Nov 21, 2023
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