Skip to content

Build the doc in a seperate folder then move it#16020

Merged
sgugger merged 14 commits intomasterfrom
doc_builder_fix_push
Mar 10, 2022
Merged

Build the doc in a seperate folder then move it#16020
sgugger merged 14 commits intomasterfrom
doc_builder_fix_push

Conversation

@sgugger
Copy link
Copy Markdown
Collaborator

@sgugger sgugger commented Mar 9, 2022

What does this PR do?

To fix the issue pointed out in #16019 this removes the need for stashing by:

  • building the doc in a folder outside any repos
  • pull the distant repos once the doc build is finished
  • then move other the built doc to those repos and push

TODO: the dev job as well once the main job has been tested

@sgugger sgugger requested review from LysandreJik and mishig25 March 9, 2022 16:05
Comment on lines +91 to +93
git pull &&
mv ../builder_dir/transformers . &&
git status &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when I tried to replicate locally, I get this error

mishig$ pwd
/Users/mishig/Desktop/moon/moon-landing/repos/doc-build
mishig$ mv ~/Desktop/build_dir/transformers/ .
mv: rename /Users/mishig/Desktop/build_dir/transformers/ to ./transformers/: Directory not empty

echo "No diff in the documentation."
fi &&
git pull &&
mv ../builder_dir/transformers . &&
Copy link
Copy Markdown
Contributor

@mishig25 mishig25 Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
mv ../builder_dir/transformers . &&
rm -rf ./transformers/master &&
mv ../builder_dir./transformers &&

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Tried with cp -r but will use this if it fails.

Copy link
Copy Markdown
Contributor

@mishig25 mishig25 Mar 9, 2022

Choose a reason for hiding this comment

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

on each build, the bundled js gets a unique name here as start-{ID}.js (compare it to dtsts verions where there is only one start-xyz.js file)

unlike rm -rf, with cp -r, they will pile up (although they would not cause any rendering issue; will just look noisy)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will adjust! Good point!

Comment thread .github/workflows/build_documentation.yml Outdated
Comment thread .github/workflows/build_documentation.yml Outdated
@mishig25 mishig25 mentioned this pull request Mar 9, 2022
5 tasks
Copy link
Copy Markdown
Contributor

@mishig25 mishig25 left a comment

Choose a reason for hiding this comment

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

if all the CI passes & everything look good in the respective repos, lgtm for me!

Copy link
Copy Markdown
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks for working on it @sgugger!

@sgugger sgugger merged commit 1059139 into master Mar 10, 2022
@sgugger sgugger deleted the doc_builder_fix_push branch March 10, 2022 12:44
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.

3 participants