-
Notifications
You must be signed in to change notification settings - Fork 236
(GH-799) Revive docfx scripts #800
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
Conversation
Note that this does not actually fix docfx, but at least makes it so it can be executed. Building with the 1.x docfx gives
While the 2.x docfx errors with
|
41d340f
to
9156e6c
Compare
9156e6c
to
bd55b82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to try running this before approving, but it looks really good to me :) Thanks @glennsarti!
PowerShellEditorServices.build.ps1
Outdated
|
||
Write-Host "`n### Using dotnet SDK at path $SDKVersionPath" -ForegroundColor Green | ||
} else { | ||
Throw "Unable to find any SDKs in path $SDKPath" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep Throw
as lowercase just since all the other keywords we use are lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
scripts/BuildDocs.ps1
Outdated
@@ -11,18 +11,47 @@ $docsRepoPath = [System.IO.Path]::GetFullPath("$PSScriptRoot\..\docs\_repo") | |||
# Ensure the tools path exists | |||
mkdir $toolsPath -Force | Out-Null | |||
|
|||
if (![System.IO.File]::Exists($docfxBinPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not Test-Path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That will be more tolerant of xplat paths)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise worth a comment as to why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe wrap with a function that resolves the path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copying the code style of
if (![System.IO.File]::Exists($docfxExePath)) { |
I defer to having a consistent style (even if wrong) than inconsistent. Do you want me to change both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think change both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed both.
scripts/BuildDocs.ps1
Outdated
# 2.1 support seems to be slated for docfx 3.x release - https://github.com/dotnet/docfx/projects/1 | ||
# | ||
# For now use the 1.x series of docfx | ||
(new-object net.webclient).DownloadFile('https://github.com/dotnet/docfx/releases/download/v1.9.4/docfx.zip', "$docfxZipPath") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not using Invoke-WebRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea. This is just a whitespace and version update of the original code from @daviwil 3 years ago. https://github.com/PowerShell/PowerShellEditorServices/blame/f847e2fd64f89edcc089803dd03775409e511344/scripts/BuildDocs.ps1#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, up to you, but Invoke-WebRequest
would be my preference here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the age, it was probably due to IWR being SOO slow compared to WebClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there were (are) issues with IWR in the Windows PowerShell and early versions of PowerShell Core. Do we still need to build on a system running PS 5.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Changed to IWR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah didn't know about issues. Early versions of PSCore won't be a problem since they're out of support in a few months. WinPS I have no idea, but performance shouldn't be an issue for official builds at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on WinPS (I haven't made the change PSCore yet ⏲).
e7e0485
to
4ce1676
Compare
@rjmholt A reminder that this doesn't fix the documentation problem, this just gets us to a point where we can start trying to fix it. So IMO, cross platform support of the documentation generation isn't a priority right now. |
Understood. It should run on Windows though, right? |
Previously the document generation scripts for docfx were broken e.g. Unable to download docfx from github due to TLS. This commit; * Updates the BuildDocs script to properly clean and download docfx from Github * Updates the docfx.json file to use the correct naming (cwd -> src) and to use the correct project files * Updates gitignore to ignore docfx working directory * Adds two build tasks (BuildDocs and ServeDocs) to build and serve autogenerated documentation * Adds additional configuration switch for Invoke-Build for legacy applications, like docfx, which use MSBuild as a library but it needs the dotNet Core versions of MSBuild instead of VS 2015/2017
This commit removes redundant code from the build scripts.
4ce1676
to
570d0c6
Compare
Yup! PowerShell 6 (and probably Windows PowerShell) I still haven't figured out the magical environment for docfx to work yet. But that's for another PR.. |
I'll see if I can track down someone to fill me in on docfx. I know the PowerShell team is standardising on markdown instead, but I'm guessing docfx has a more structured website integration story. |
Actually that's a really good point. What's https://github.com/PowerShell/PowerShell using for automated docs creation? |
PowerShell's docs are kept in the PowerShell-Docs repo. That stores all the docs as markdown files and builds them with platyPS, but I think it's reasonably specific to PowerShell — not meant for .NET code. |
Here's an example: MicrosoftDocs/PowerShell-Docs#3238 |
I meant the documentation of C# classes of PowerShell e.g. Where does this information go? |
I've spoken to @SteveL-MSFT and @joeyaiello and the way this works is the PowerShell SDK gets published on nuget and the documentation XML gets pulled from the package and semi-automatically is published to docs.microsoft.com. After discussing with them, publishing to nuget seems to be the way we want to go. It will take a little bit longer, but should make API documentation much easier to discover and keep it up to date once we integrate nuget publishing into our release process. |
I've reopened #41 for this. |
@rjmholt The docs for the dotnet classes in PowerShell are in MicrosoftDocs/powershell-docs-sdk-dotnet. This content is autogenerated from the nuget and XML. This content must be updated in the source code (/// comments) and new nupkg/XML created. Changes made in this repo get overwritten by the autogeneration processes. |
Yes understood. Thanks @sdwheeler |
Ahh, this is very out of date, I need to close it, sorry! Thanks anyway for the work. I am looking into setting up a GitHub Action to run |
Previously the document generation scripts for docfx were broken e.g. Unable to
download docfx from github due to TLS. This commit;
use the correct project files
autogenerated documentation
like docfx, which use MSBuild as a library but it needs the dotNet Core
versions of MSBuild instead of VS 2015/2017