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

RtD docs previews: Cancel building PRs if no changes in Doc dir #104100

Merged
merged 10 commits into from
May 24, 2023

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented May 2, 2023

As suggested by @humitos at #103843 (comment), cancel building pull requests when there aren't changes in the Doc directory.

If there are no changes (git diff exits with 0) we force the command to return with 183. This is a special exit code on Read the Docs that will cancel the build immediately.

https://docs.readthedocs.io/en/stable/build-customization.html#cancel-build-based-on-a-condition

@hugovk
Copy link
Member Author

hugovk commented May 2, 2023

For the first commit:

Problem in your project's configuration. Invalid "build.commands": .readthedocs.yml: The keys build.jobs and build.commands can't be used together.

https://readthedocs.org/projects/cpython-previews/builds/20362322/

For the second:

We encountered a problem with a command while building your project. To resolve this error, double check your project configuration and installed dependencies are correct and have not changed recently.
...
/bin/sh: 1: if: not found
/bin/sh: 2: Syntax error: "then" unexpected

https://readthedocs.org/projects/cpython-previews/builds/20362458/

@humitos Any suggestions? Thank you!

@Eclips4
Copy link
Member

Eclips4 commented May 2, 2023

Just guessing, maybe move all commands and lines of bash code into jobs/post_checkout section will help?

.readthedocs.yml Outdated Show resolved Hide resolved
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@sobolevn
Copy link
Member

Hm, looks like sh cannot handle ] && expr case. Better use [ ] twice.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Sorry, I should have posted the full version right away :)

.readthedocs.yml Outdated Show resolved Hide resolved
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@humitos
Copy link
Contributor

humitos commented May 15, 2023

Hi! Sorry for the late reply. I've been on vacations and then Write the Docs conference after that.

I was 100% sure that we already solved the problem you are facing, but I just found that we reverted the change because it was causing other issues (readthedocs/readthedocs.org#10206). I'm sorry for the confusion here.

As @Eclips4 commented, putting this into a script and calling it from build.jobs.post_ should work as a workaround.

@humitos
Copy link
Contributor

humitos commented May 15, 2023

Interesting... It seems it works fine with build.jobs (I tested it again at https://readthedocs.org/projects/test-builds/builds/20716439/) but it doesn't with build.commands for some reason. I will need to keep researching about this and I will come back, hopefully, with a solution 😄

@sobolevn
Copy link
Member

It works locally, maybe this is some kind of yaml issue?

sh-3.2$ if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && [ "$(git diff --quiet origin/main -- Doc/ .readthedocs.yml; echo $?)" -eq 0 ]; then
>   echo 1
> fi
sh-3.2$ 

@sobolevn
Copy link
Member

Maybe like this?

- if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && [ "$(git diff --quiet origin/main -- Doc/ .readthedocs.yml; echo $?)" -eq 0 ]; then \
    echo "No changes to Doc/ - exiting the build."; \
    exit 183; \
  fi

@sobolevn
Copy link
Member

I have no idea what is going on :)
Maybe we are missing something obvious?

@hugovk hugovk marked this pull request as draft May 21, 2023 20:03
@humitos
Copy link
Contributor

humitos commented May 23, 2023

Today we are deploying a change on Read the Docs that should fix this. Hopefully, it works 🤞🏼

@humitos
Copy link
Contributor

humitos commented May 23, 2023

The fix on Read the Docs is already deployed and my tests worked. Can you confirm that it works for you now as well? Note that you have to adapt your command to revert it to the state it was before (multi-line). I will do a suggestion on the PR.

.readthedocs.yml Outdated Show resolved Hide resolved
arhadthedev and others added 3 commits May 23, 2023 22:37
@hugovk hugovk marked this pull request as ready for review May 23, 2023 19:27
@hugovk hugovk merged commit c3204ed into python:main May 24, 2023
@hugovk hugovk deleted the rtd-doc-dir-only branch May 24, 2023 13:54
@hugovk
Copy link
Member Author

hugovk commented May 27, 2023

This is working quite well, thanks!

One thing though, for example on #105008 (comment), the build was cancelled as expected:

Details image

https://readthedocs.org/projects/cpython-previews/builds/20835824/

However, the status check on GitHub still shows as pending:

Details image

And also shows in the tab:

Details image

Would it be possible to have it report as skipped instead of pending?

@humitos
Copy link
Contributor

humitos commented May 29, 2023

Hi @hugovk. This should be reporting success (✅) to GitHub when the build is skipped. It seems we introduced a bug here or something that broke this behavior. If you have the time, please open an issue in https://github.com/readthedocs/readthedocs.org/

@hugovk
Copy link
Member Author

hugovk commented May 29, 2023

Thanks, reported: readthedocs/readthedocs.org#10364


Another PR had no Doc/ or .readthedocs.yml changes but still built the docs preview:

We can see the check was there but returned 0:

if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && [ "$(git diff --quiet origin/main -- Doc/ .readthedocs.yml; echo $?)" -eq 0 ];
then
  echo "No changes to Doc/ - exiting the build.";
  exit 183;
fi

Any ideas why this didn't skip?

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.

6 participants