-
Notifications
You must be signed in to change notification settings - Fork 3k
Docs: improve local editing experience #13664
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
|
thank you for working on this! could you also try to benchmark how long |
|
Hi @kevinjqliu,
These results demonstrate a ~53 % acceleration in our docs‑serve delivery pipeline, significantly reducing the turnaround for iterative doc previews. Tested on macOS (M1 Pro, Python 3.8.2). No functional regressions observed. Let me know if you’d like any additional profiling data or comparisons on other platforms. |
|
Hi @kevinjqliu 👋 Thanks for reviewing this enhancement. Unless you have any blocking concerns, I’m ready for approval and merge. Would you mind giving this PR a 👍 and approving/merging it when you have a moment? Appreciate your time! 🚀 |
|
Hi @electrum / @kevinjqliu / @martint, Appreciate your time! |
site/dev/common.sh
Outdated
| then | ||
| /usr/bin/sed -i '' -E "s/(^site\_name:[[:space:]]+docs\/).*$/\1${ICEBERG_VERSION}/" ${ICEBERG_VERSION}/mkdocs.yml | ||
| /usr/bin/sed -i '' -E "s/(^[[:space:]]*-[[:space:]]+Javadoc:.*\/javadoc\/).*$/\1${ICEBERG_VERSION}/" ${ICEBERG_VERSION}/mkdocs.yml | ||
| /usr/bin/sed -i '' -E "s/(^site\_name:[[:space:]]+docs\/).*$/${ICEBERG_VERSION}/" ${ICEBERG_VERSION}/mkdocs.yml |
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.
whi is this changed?
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.
You’re right. Those sed replacements were corrupted — the \1 backreference ended up as a control char (^A), and I mistakenly escaped site_name. I’ll fix both Darwin and Linux blocks to consistently use \1${ICEBERG_VERSION} and quote paths. No behavior change beyond correcting the bug.
site/dev/common.sh
Outdated
| # Modify .md files to exclude versioned documentation from search indexing | ||
| python3 -c "import os | ||
| for f in filter(lambda x: x.endswith('.md'), os.listdir()): lines = open(f).readlines(); open(f, 'w').writelines(lines[:2] + ['search:\n', ' exclude: true\n'] + lines[2:]);" | ||
| for f in filter(lambda x: x.endswith('.md'), os.listdir()): lines = open(f).readlines(); open(f, 'w').writelines(lines[:2] + ['search: |
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.
what's changed 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.
Nothing editor messed up in multiple places where some newline characters are replaced with an actual new line.
fixed in recent commit, Thanks for reviewing.
| @@ -15,12 +15,17 @@ | |||
|
|
|||
| .PHONY: help | |||
| help: # Show help for each of the Makefile recipes. | |||
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.
why is this changed?
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.
Good catch. That change was unintentional—my editor converted \033 into literal ESC bytes and the line also lost the \n. I’ll revert to a portable version. To avoid CI/non-TTY issues, I’ll drop raw colors and keep plain output so this PR stays focused on the local docs fast-path.
site/dev/common.sh
Outdated
| echo "Latest version is: ${latest_version}" | ||
|
|
||
| # Create the 'latest' version of documentation | ||
| create_latest "${latest_version}" |
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.
Do we need to create the latest version? I think we only work on nightly version.
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’s a good point: for the fast-path (--local) build, we don’t actually need to generate the latest version. Skipping it speeds things up. I’ll update the local path so it only creates (or symlinks) the nightly version, leaving the full latest creation only in normal builds.
…quote paths) - Fixed sed replacements in site/dev/common.sh: * Restored correct \1 backreferences (removed stray ^A control chars). * Corrected `site_name` key (removed accidental escape). * Quoted mkdocs.yml paths for safety. * Made Darwin and Linux blocks consistent. No functional change beyond correcting the replacements. This addresses the reviewer comment on unexpected edits in common.sh.
Replying to comment, https://github.com/apache/iceberg/pull/13664/files#r2278265126: That’s a good point: for the fast-path (--local) build, we don’t actually need to generate the latest version. Skipping it speeds things up. I’ll update the local path so it only creates (or symlinks) the nightly version, leaving the full latest creation only in normal builds.
site/dev/common.sh
Outdated
|
|
||
| # Update version information within the mkdocs.yml file using sed commands | ||
| if [ "$(uname)" == "Darwin" ] | ||
| # Update version information within the mkdocs.yml file using sed commands |
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.
Nit: extra indent
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 it, removed extra indent
site/dev/common.sh
Outdated
| # Update version information within the mkdocs.yml file using sed commands | ||
| if [ "$(uname)" == "Darwin" ] | ||
| # Update version information within the mkdocs.yml file using sed commands | ||
| if [ "$(uname)" = "Darwin" ]; |
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.
do we need to change this part?
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 necessary, fixed it
|
@manuzhang Could you please take another look and let me know if it’s good to approve? |
site/dev/common.sh
Outdated
| /usr/bin/sed -i '' -E "s/(^site\_name:[[:space:]]+docs\/).*$/\1${ICEBERG_VERSION}/" ${ICEBERG_VERSION}/mkdocs.yml | ||
| /usr/bin/sed -i '' -E "s/(^[[:space:]]*-[[:space:]]+Javadoc:.*\/javadoc\/).*$/\1${ICEBERG_VERSION}/" ${ICEBERG_VERSION}/mkdocs.yml | ||
| elif [ "$(expr substr $(uname -s) 1 5)" == "Linux" ] | ||
| /usr/bin/sed -i '' -E "s/(^site_name:[[:space:]]+docs\/).*$/\1${ICEBERG_VERSION}/" "${ICEBERG_VERSION}/mkdocs.yml" |
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.
@yeswanth-s1th can you check this change again?
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 rechecked the block and made a small adjustment: on Linux I kept the original token-level pattern ([^[:space:]]+) so only the version number gets replaced and nothing else on the line is touched. On macOS and for the Javadoc lines I left them as they are in main (.*$) so behavior there doesn’t change. I also added quotes around $(uname -s) for safety. This keeps the change minimal but addresses the concern you raised.
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.
@yeswanth-s1th have you checked this?
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.
Thanks for following up. Yes, I’ve rechecked this.
On Linux, I kept the original token-level pattern ([^[:space:]]+) so it only replaces the version number after docs/ without affecting anything else on the line. On macOS and for the Javadoc lines, I left them as they are in main (.*$) so their behavior doesn’t change.
I also added quotes around $(uname -s) for safety. This keeps the behavior consistent with main while addressing the earlier concern, and keeps the change minimal.
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.
But I don't think the change is related to this PR. I'd prefer having the enhancement in a separate PR.
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.
@manuzhang
Makes sense. I’ve reverted this block to match main so the PR stays focused. I’ll open a separate PR for the small sed/quoting cleanups.
Fix sed patterns in mkdocs.yml update script - Keep Linux site_name pattern token-level ([^[:space:]]+) so only the version number is replaced, avoiding clobbering the rest of the line. - Leave Darwin and Javadoc patterns unchanged (.*$) to match main. - Add quotes around $(uname -s) for safety. Keeps behavior consistent with main while addressing reviewer concern with minimal changes.
@manuzhang Could you please take another look and let me know if it’s good to approve? |
Just following up — Could you take another look when you get a chance? Let me know if anything else needs to be addressed. Thanks! |
|
@yeswanth-s1th have you checked my latest comments? |
Yes, I’ve replied in the thread with the details. |
reverting this block to match main so the PR stays focused. I’ll open a separate PR for the small sed/quoting cleanups.
|
@yeswanth-s1th could you please rebase on latest main branch? |
|
@yeswanth-s1th local build failed for me. Could you please check? |
|
hey @yeswanth-s1th thanks for working on this! I tried to run this locally too and got the same error I took a look at the code path for I think this PR is on the right track; for local rendering, we just need the "latest" version of the docs. |
Thanks for letting me know I will work on this weekend and try to fix the issue. Thanks |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
hey @yeswanth-s1th thanks for working on this. I gave it a try locally and was not able to run I think I found 2 major causes for the slowdown, please take a look at #14267 (review) |
|
closing this in favor of #14267. Thanks again for working on it. please try out the new
|
closes #13661
docs: improve local editing experience
This commit introduces a new
serve-localcommand to the Makefile that allows for a much faster local build of the documentation.The
serve-localcommand works by creating symbolic links to the localdocsandjavadocdirectories instead of pulling all the versioned documentation from the remote repository. This significantly reduces the build time, making it much easier to edit and preview the documentation locally.The following changes were made:
pull_local_docsfunction was added tosite/dev/common.shthat creates symbolic links to the localdocsandjavadocdirectories.site/dev/setup_env.shscript was modified to include a--localflag that triggers the use of thepull_local_docsfunction.site/dev/serve.shscript was modified to accept and pass on the--localflag.serve-localcommand was added to thesite/Makefilethat executesserve.shwith the--localflag.