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

Clean up contributing docs and use RTD for pull request previews #1798

Merged
merged 16 commits into from
Aug 5, 2024

Conversation

stevepiercy
Copy link
Contributor

@stevepiercy stevepiercy commented Jul 27, 2024

This PR cleans up contributing documentation for Plone 6 and switches from Netlify to Read the Docs for pull request previews.

Here's the changes to preview on a separate branch:

https://plonerestapi.readthedocs.io/en/docs-contributing/contributing/index.html

Pull request previews won't build until merged to main.

@mister-roboto
Copy link

@stevepiercy thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

Copy link

netlify bot commented Jul 27, 2024

Deploy Preview for plone-restapi failed. Why did it fail? →

Name Link
🔨 Latest commit ef8ce84
🔍 Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/66a572316480fe00086ca5ca

@stevepiercy stevepiercy changed the title Clean up contributing docs Clean up contributing docs and use RTD for pull request previews Jul 27, 2024
@stevepiercy
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor Author

I'm stuck. docutils dropped support for Python 3.8. Zope pins it at 0.18.1, but we need 0.21.2 to use modern Sphinx and themes. How should I handle this?

I could create a new plone-6.0.x-python3.8.cfg file without the pin, and put a conditional in tests.yml for the run command.

run: if [ "${{ matrix.plone-version }}" == "6.0" ] && [ ${{ matrix.python-version }} == '3.8' ]; then buildout -t 10 -c plone-6.0.x-python3.8.cfg; else buildout -t 10 -c plone-${{ matrix.plone-version }}.x.cfg; fi;

I'll try that.

@stevepiercy
Copy link
Contributor Author

@jenkins-plone-org please run jobs

.readthedocs.yaml Outdated Show resolved Hide resolved
Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

Two inline comments, for the rest this seems fine. Thanks!

@stevepiercy
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor Author

Thanks for the review! Merging!

@stevepiercy stevepiercy merged commit 602ccc6 into main Aug 5, 2024
24 checks passed
@stevepiercy stevepiercy deleted the docs-contributing branch August 5, 2024 19:01
@davisagli
Copy link
Member

I'm not happy with overriding a Zope pin here. Like I said in zopefoundation/Zope#1220 (comment), I think a better solution is to pin readme-renderer to a version that's compatible with the version of Sphinx that we use.

@stevepiercy
Copy link
Contributor Author

@davisagli there's a ton of changes coming through with Sphinx 8, MyST Parser 4.0, plone-sphinx-theme, and other packages, all of which depend on docutils > 21. Agreed, this is not ideal, but I need to start moving forward and not wait around.

Zope pins docutils for the sole reason to use RTD Sphinx Theme. Personally I believe that is not a good reason to accept its choice. Other Sphinx themes have leapt far beyond RTD Sphinx Theme. I'll ask if they want to move to a modern Sphinx theme.

@davisagli
Copy link
Member

@stevepiercy Sure, I support moving forward with a newer version of docutils. But we have to do it in the right order and test it first with plone.restapi's dependencies. If changing this pin here breaks something in another Plone package, we would have no way of knowing. In Plone docutils is not used only for documentation, but also for transforms between rst and html within Plone.

The main thing I'm saying is that we should update the pin first in buildout.coredev. That will allow us to see if it breaks tests in any Plone package. If not, then we can continue like this.

@stevepiercy
Copy link
Contributor Author

In Plone docutils is not used only for documentation, but also for transforms between rst and html within Plone.

The main thing I'm saying is that we should update the pin first in buildout.coredev.

TIL.

I'll create a PR for CI tests, and post back here.

@stevepiercy
Copy link
Contributor Author

Well, I'm stuck. Neither Plone 6 or 5 docs say how to pin a package in buildout.coredev. I made some incorrect guesses. Which of the various files should I edit?

Oh, and should I run either ./bin/buildout or ./bootstrap.sh after a change, correct?

@davisagli
Copy link
Member

davisagli commented Aug 5, 2024 via email

stevepiercy added a commit to plone/buildout.coredev that referenced this pull request Aug 5, 2024
@stevepiercy
Copy link
Contributor Author

Done plone/buildout.coredev#942. Thanks for the assist @davisagli. So far things are passing, and I just started Jenkins for the two builds on 6.1.

Should I do another PR against 6.0?

@davisagli
Copy link
Member

Should I do another PR against 6.0?

@stevepiercy I think that makes sense.

stevepiercy added a commit to plone/buildout.coredev that referenced this pull request Aug 6, 2024
Zope uses an older version of docutils for its Sphinx theme, which Plone does not use.

See plone/plone.restapi#1798
@stevepiercy
Copy link
Contributor Author

stevepiercy commented Aug 6, 2024

Opened similar PR, but had to use docutils 0.20.1 to get Python 3.8 support. 😞

plone/buildout.coredev#943

@mauritsvanrees
Copy link
Member

If it helps anywhere, I could update the docutils pin in https://dist.plone.org/release/6.1-dev/ and https://dist.plone.org/release/6.0-dev/
Let me know if you see CI tests failing in Plone packages that would be fixed by such a change.

@stevepiercy
Copy link
Contributor Author

@mauritsvanrees can you explain how it might help? I sincerely don't know the details. My guess is that it would give developers an opportunity to test a build using a *-dev release before trying a full 6.x release.

I want to do the wisest thing, including reverting pins and putting them in the right places. What do you recommend?

We now have three pins referenced in this PR. Should I remove the one that I did in this PR, now that the other two were merged?

Please let me know. Thank you!

@stevepiercy
Copy link
Contributor Author

@mauritsvanrees @davisagli see https://github.com/zopefoundation/Zope/pull/1221/files where they switched to the Furo theme and use pinned versions of docutils according to the versions of Python they support.

Is there anything I should undo from the three PRs?

@mauritsvanrees
Copy link
Member

The idea would be that if there is another Plone package that would benefit from the new docutils version for its docs, then it may help if that new docutils version is on dist.plone.org/release/{6.0,6.1}-dev. plone.restapi here is extending the 6.0.11.1 versions/constraints, so it does not benefit.

Anyway, I have updated those two dev release directories with new docutils and two other packages.

Nothing to undo for now. At one point when Plone coredev starts using a new Zope release with the updated version, then we can remove the overrides.

@stevepiercy
Copy link
Contributor Author

At one point when Plone coredev starts using a new Zope release with the updated version, then we can remove the overrides.

Should we create an issue in the appropriate repo, whichever that is, to remind us?

@mauritsvanrees
Copy link
Member

No, once there is a new version of Zope released, I will adjust the coredev-buildouts and then I always look if we have set any overrides. You correctly added the pin under the OVERRIDES headers, so that will work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants