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

Simplify doc build process by removing empty directories locally #38185

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Jun 10, 2024

Now doc builder removes empty directories only in its own source directory.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@kwankyu kwankyu changed the title p/locally remove empty directory Simplify doc build process by removing empty directories locally Jun 10, 2024
@kwankyu kwankyu mentioned this pull request Jun 10, 2024
5 tasks
Copy link

github-actions bot commented Jun 10, 2024

Documentation preview for this PR (built with commit cb0da58; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 12, 2024
…ories locally

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

and thus hopefully fixes the mysterious issue
sagemath#37858 (comment).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

sagemath#37858

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38185
Reported by: Kwankyu Lee
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
…ories locally

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

and thus hopefully fixes the mysterious issue
sagemath#37858 (comment).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

sagemath#37858

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38185
Reported by: Kwankyu Lee
Reviewer(s):
@vbraun
Copy link
Member

vbraun commented Jun 16, 2024

Bootstrap download of the confball does not work (note the missing version):

Edit: wrong ticket, this is from #37430

@vbraun
Copy link
Member

vbraun commented Jun 16, 2024

Needs work because it has #37858 merged in

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 26, 2024

This PR is now separate from #37858. Back to positive review?

@kwankyu kwankyu force-pushed the p/locally-remove-empty-directory branch from a50dc92 to cb0da58 Compare August 4, 2024 10:44
@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 4, 2024

Has the code that removes empty directories been merged already?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 4, 2024

Before this PR, empty directories in all docs (each of them is a sphinx project) are removed at the same time when references (BIBLIO in src/doc/Makefile, one particular sphinx project) is made. This approach works but violates , well, the principle of separation of concerns(?).

With this PR, empty directories in each doc are removed when that doc is made. So one sphinx project does not touch empty directories in governance of another sphinx project. This approach also works and looks better.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 5, 2024

Right, that's what I understood from the PR description, but in the diff I only see that --no-prune-empty-dirs is no longer used.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 5, 2024

In the phase of building inventory, --no-prune-empty-dirs is not used, so that empty directories are removed. In the phase of building html files, --no-prune-empty-dirs is used (as before) so that no time is wasted to remove (not existing anymore) empty directories.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 5, 2024

Thanks!

@vbraun vbraun merged commit a1deb75 into sagemath:develop Aug 10, 2024
21 of 23 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants