Skip to content

[GitHub] Add basic CI for libclang Python binding unit tests #76784

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 6, 2024
Merged

[GitHub] Add basic CI for libclang Python binding unit tests #76784

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fffedc0
[GitHub] Add CI for libclang Python unit tests
linux4life798 Dec 30, 2023
8fd0cac
[GitHub] Remove libclang Python actions fork protection
linux4life798 Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/libclang-python-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: Libclang Python Binding Tests

permissions:
contents: read

on:
workflow_dispatch:
push:
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to add branches: - main here. This action now runs whenever I push a branch to my fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. That's why I was seeing the job run in places it shouldn't be. Fixed in c41472d. Thanks for pointing this out!

Copy link
Contributor Author

@linux4life798 linux4life798 Jan 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm. What's the issue with the applicable test running in forks?

I intentionally didn't add these restrictions to allow for easier testing of changes in forks. I figured if you didn't like a particular workflow, then you just disable the specific workflow using the GitHub UI. No code modified. Plus, I believe all workflows are disabled by default for new forks (using a different mechanism), as I called out in 8fd0cac.

The alternative, where we disable them from within the yaml file, doesn't seem great to me. This means that you have to maintain additional commits/changes in every branch of your fork to re-enable this behavior. When you submit a PR, you have to remove these temporary commits. This discourages local CI testing.

Also, I think having pre-commit/PR testing is pretty important to ensure that the person making the change sees/corrects the failure, rather than maintainers seeing the failure after commit. The main branch tests are important to maintain a global cache (and of course to verify of the tree).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@linux4life798 How do you disable specific workflows using the GitHub UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@linux4life798 How do you disable specific workflows using the GitHub UI?

On the particular action/workflow you want to disable, you click the three dots in the top right and click Disable workflow:
image

After that, it will show a yellow banner warning that the workflow is disabled:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for now we need to continue to restrict our workflows to the llvm organization or the llvm/llvm-project repository. I agree with you, though, that doing this makes testing harder and is inconvenient for forks that actually do want to test. On the other hand asking all the thousands of forks to manually disable the workflows doesn't seem like it scales well.

I don't know which solution is best, but we have had people complain in the past about unwanted workflows running in their forks. I'm not sure if they were aware of this option, though.

If we want to change our policy and remove the llvm org and llvm/llvm-project repo restrictions from our workflows, then I think we need an RFC to see how the rest of the community and the fork owners feel about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being automatically disabled is referring to scheduled workflows. This is not a scheduled workflow.

When the workflow is setup to also work on pushes anywhere, it runs on PRs it is not supposed to (at least from what I can tell).

It is more difficult to test in forks with these settings, but there are practical issues as mentioned above, and it's not really expected behavior. It's easy enough if you're working on the workflows to push another commit disabling some of the checks to make testing easier. I don't think making the tests opt-out by default on forks is a good idea.

Copy link
Contributor Author

@linux4life798 linux4life798 Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know which solution is best, but we have had people complain in the past about unwanted workflows running in their forks. I'm not sure if they were aware of this option, though.

If we want to change our policy and remove the llvm org and llvm/llvm-project repo restrictions from our workflows, then I think we need an RFC to see how the rest of the community and the fork owners feel about this.

Fair. I have certainly been annoyed by failing action results in the past.

Being automatically disabled is referring to scheduled workflows. This is not a scheduled workflow.

Ohhh, I see that in the documentation, but that wasn't what I was referring to. I guess that is yet another automatic action disablement feature.

What I'm talking about seems to be weirdly undocumented. When you fork a repo with actions, you see the following message that forces you to opt-in to running actions/workflows:

image

Am I the only one who sees this? Try yourself by forking https://github.com/repo-sync/github-sync, which only has one on-push-tags action.

When the workflow is setup to also work on pushes anywhere, it runs on PRs it is not supposed to (at least from what I can tell).

Hmm. I didn't see any spurious Action runs in llvm proper. There are some confusing runs, like https://github.com/llvm/llvm-project/actions/runs/7445527409. When you dig into it, it looks like the rationale is that a parent commit did touch one of the filtered paths. If you push a bunch of commits, it won't run the action on each individual commit. It just runs the action on the last commit, but takes into account all paths touch since the last run. This is good.

It is more difficult to test in forks with these settings, but there are practical issues as mentioned above, and it's not really expected behavior. It's easy enough if you're working on the workflows to push another commit disabling some of the checks to make testing easier. I don't think making the tests opt-out by default on forks is a good idea.

For new users, you need to opt-in to enable actions. For existing users, you can disable all actions if you think they are annoying. Otherwise, it seems like a bonus add to be able to have you code automatically checked, assuming the rules for them to run are configured properly.

To be totally honest, I still have to manually disable all of the Actions I'm not interested in, since if: github.repository_owner == 'llvm' just results in the action being canceled. Seeing all the Canceled run item in the All workflows is still too annoying.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm talking about seems to be weirdly undocumented. When you fork a repo with actions, you see the following message that forces you to opt-in to running actions/workflows:

I do see that. I didn't realize that was a thing (must be pretty recent). Most LLVM forks are old however (at least for active contributors). We could get people to enable them and I guess active contributors would be the only ones seeing it and should be able to do so relatively easily, but it is another step.

Hmm. I didn't see any spurious Action runs in llvm proper. There are some confusing runs, like https://github.com/llvm/llvm-project/actions/runs/7445527409. When you dig into it, it looks like the rationale is that a parent commit did touch one of the filtered paths. If you push a bunch of commits, it won't run the action on each individual commit. It just runs the action on the last commit, but takes into account all paths touch since the last run. This is good.

The run that you mentioned is a PR (#77112) that has absolutely nothing to do with the libclang python bindings. I think this is normally an issue with merge commits, but there wasn't even one there in that case (probably due to the force-push). Sure, it just runs on the last commit, but it never should be running on that PR in the first place because it isn't relevant and potential noise to whoever is working on it. It's also just duplicate coverage as the last time main changed with the path-based filtering, the action should have already run. From what I can tell spurious runs like this stopped after I pushed the minor fix commit. We also need to be conservative with what we do run as we don't have that much compute.

To be totally honest, I still have to manually disable all of the Actions I'm not interested in, since if: github.repository_owner == 'llvm' just results in the action being canceled. Seeing all the Canceled run item in the All workflows is still too annoying.

Sure, but I think the main issue is when actions fail and then an automatic email notification is sent out. If the actions just cancelled, nothing gets sent and no compute is spent where someone probably doesn't want/need it.

This is also probably much better discussed on Discourse rather than buried in this merged PR.

paths:
- 'clang/bindings/python/**'
- 'clang/tools/libclang/**'
- 'clang/CMakeList.txt'
- '.github/workflows/libclang-python-tests.yml'
- '.github/workflows/llvm-project-tests.yml'
pull_request:
paths:
- 'clang/bindings/python/**'
- 'clang/tools/libclang/**'
- 'clang/CMakeList.txt'
- '.github/workflows/libclang-python-tests.yml'
- '.github/workflows/llvm-project-tests.yml'

concurrency:
# Skip intermediate builds: always.
# Cancel intermediate builds: only if it is a pull request build.
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }}

jobs:
check-clang-python:
# Build libclang and then run the libclang Python binding's unit tests.
name: Build and run Python unit tests
uses: ./.github/workflows/llvm-project-tests.yml
with:
build_target: check-clang-python
projects: clang
# There is an issue running on "windows-2019".
# See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
os_list: '["ubuntu-latest"]'