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

forc doc: Generate dependency documentation #4546

Merged
merged 16 commits into from
May 22, 2023
Merged

Conversation

eureka-cpu
Copy link
Contributor

@eureka-cpu eureka-cpu commented May 6, 2023

Description

Generates the documentation for dependencies & small improvements to the API.

What's new so far:

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@eureka-cpu eureka-cpu added enhancement New feature or request forc-doc Everything related to the `forc doc` command plugin. labels May 6, 2023
@eureka-cpu eureka-cpu self-assigned this May 6, 2023
@eureka-cpu eureka-cpu marked this pull request as ready for review May 15, 2023 21:20
@eureka-cpu eureka-cpu requested a review from a team May 15, 2023 21:20
@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented May 15, 2023

Not sure why the CI build for std can't find the canonical path for core. cc @kayagokalp do you have an idea? I could just add the --no-deps flag to quiet it, but then I'm not sure we are getting the most out of that test.

also cc @mitchmindtree & @JoshuaBatty

@kayagokalp
Copy link
Member

Not sure why the CI build for std can't find the canonical path for core. cc @kayagokalp do you have an idea?

Sorry @eureka-cpu, couldn't get to this today, will try to understand first thing tomorrow though 👍

Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

I tracked down the issue, if you print the path in the build_deps function you can see that before canonicalizing it is ../sway-lib-core but the relative path should be taken into account from sway-lib-std's folder.

CI is running the command like forc-doc --manifest-path ../sway-lib-std in the root folder which tries to canonicalize /sway/../sway-lib-core which does not exist. You need to join the path of the dependency with the package's path (the package that declares the dependency).

You can verify this is the case by going into the sway-lib-std folder and running forc-doc directly there. It is working as /sway/sway-lib-std/../sway-lib-core is a valid path.

That being said, I see that forc-doc has a custom logic for building which requires bunch of common code with forc-pkg. I guess the reason for not using forc-pkg was the fact that forc-doc requires type and decl engines. I would suggest adding a nice function to forc-pkg, which uses all the established logic likeBuildPlan like our build_with_opts end point, but at the same time accepts the engines from caller. This way you would loose duplicated building steps, automatically support dependencies declared by git and ipfs as well, also this path issues will be gone as forc-pkg can give you an iterator of correct paths for each member in the BuildPlan and finally you would get the recursive dependency logic for free.

@sdankel
Copy link
Member

sdankel commented May 17, 2023

Could you add some examples of how this works to this PR description?

I looked at Address since it implements Eq from core, but there is no link to core. Is that expected?

image

I also looked at functions in the low_level_call module that use Bytes from core, and there are no links to core.

image

@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented May 17, 2023

Could you add some examples of how this works to this PR description?

I looked at Address since it implements Eq from core, but there is no link to core. Is that expected?

image

I also looked at functions in the low_level_call module that use Bytes from core, and there are no links to core.

image

Hey @sdankel yes, this is expected. This PR simply allows the dependencies to be built at all, but it does not yet implement the logic to link them together.

If you take a look at the file structure in out/doc you should now notice 3 changes when building.

  1. Each program has its own folder. So the index.html used to look something like: out/doc/index.html, but is now out/doc/program_name/index.html.
  2. assets is now static.files. This removes the probability of potential conflicts in naming conventions for programs and the assets directory.
  3. The doc directory is cleared before rebuilding the new docs. This prevents strange artifacts from appearing from older versions of forc doc.

These are small changes, but are directly related when put in the context of generating dependency docs.

@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented May 17, 2023

I tracked down the issue, if you print the path in the build_deps function you can see that before canonicalizing it is ../sway-lib-core but the relative path should be taken into account from sway-lib-std's folder.

CI is running the command like forc-doc --manifest-path ../sway-lib-std in the root folder which tries to canonicalize /sway/../sway-lib-core which does not exist. You need to join the path of the dependency with the package's path (the package that declares the dependency).

You can verify this is the case by going into the sway-lib-std folder and running forc-doc directly there. It is working as /sway/sway-lib-std/../sway-lib-core is a valid path.

That being said, I see that forc-doc has a custom logic for building which requires bunch of common code with forc-pkg. I guess the reason for not using forc-pkg was the fact that forc-doc requires type and decl engines. I would suggest adding a nice function to forc-pkg, which uses all the established logic likeBuildPlan like our build_with_opts end point, but at the same time accepts the engines from caller. This way you would loose duplicated building steps, automatically support dependencies declared by git and ipfs as well, also this path issues will be gone as forc-pkg can give you an iterator of correct paths for each member in the BuildPlan and finally you would get the recursive dependency logic for free.

Nice, I will take a look 🙂 Thanks @kayagokalp

Odd that it is working as intended on my machine though!

@kayagokalp
Copy link
Member

Odd that it is working as intended on my machine though!

Are you trying in the sway repo's root folder? It doesn't work in my system if I run forc-doc --manifest-path ./sway-lib-std, but if i go into sway-lib-std and run forc doc, it works.

@eureka-cpu
Copy link
Contributor Author

Odd that it is working as intended on my machine though!

Are you trying in the sway repo's root folder? It doesn't work in my system if I run forc-doc --manifest-path ./sway-lib-std, but if i go into sway-lib-std and run forc doc, it works.

Ah I see now, that is what I was doing. Thanks again @kayagokalp 🙂

sdankel
sdankel previously approved these changes May 19, 2023
@eureka-cpu eureka-cpu requested a review from a team May 19, 2023 20:21
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Looks nice! left couple of nits. I opened #4573, after that we can remove all the custom build effort for forc-doc, and use the usual route of forc-pkg 👌

forc-plugins/forc-doc/src/doc/mod.rs Outdated Show resolved Hide resolved
forc-plugins/forc-doc/src/main.rs Show resolved Hide resolved
forc-plugins/forc-doc/src/render/sidebar.rs Outdated Show resolved Hide resolved
@eureka-cpu eureka-cpu requested review from kayagokalp and a team May 22, 2023 15:32
@eureka-cpu eureka-cpu merged commit e5d3185 into master May 22, 2023
26 checks passed
@eureka-cpu eureka-cpu deleted the eureka-cpu/4533 branch May 22, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc-doc Everything related to the `forc doc` command plugin.
Projects
None yet
3 participants