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

Non-main changes are sent to llvm-commits #71185

Closed
nikic opened this issue Nov 3, 2023 · 13 comments · Fixed by llvm/llvm-admin#7
Closed

Non-main changes are sent to llvm-commits #71185

nikic opened this issue Nov 3, 2023 · 13 comments · Fixed by llvm/llvm-admin#7
Assignees
Labels
infrastructure Bugs about LLVM infrastructure

Comments

@nikic
Copy link
Contributor

nikic commented Nov 3, 2023

It looks like changes to spr-managed branches currently send emails to llvm-commits, which they probably shouldn't. An example is https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20231030/1235264.html.

@joker-eph fyi

@nikic nikic added the infrastructure Bugs about LLVM infrastructure label Nov 3, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/issue-subscribers-infrastructure

Author: Nikita Popov (nikic)

It looks like changes to spr-managed branches currently send emails to llvm-commits, which they probably shouldn't. An example is https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20231030/1235264.html.

@joker-eph fyi

@joker-eph
Copy link
Collaborator

I noticed when the email was sent and forwarded to @tstellar and Anton, I don't know where is the code that handle the mailing-list?

@nikic
Copy link
Contributor Author

nikic commented Nov 3, 2023

@joker-eph
Copy link
Collaborator

Thanks! That seems like the right repo, but I suspect the code the commit mailing list is https://github.com/llvm/llvm-admin/blob/main/AWS/Lambda/CommitEmailer/commit_emailer.py#L131C19-L131C19 instead?

@joker-eph
Copy link
Collaborator

There is a branch detection here: https://github.com/llvm/llvm-admin/blob/main/AWS/Lambda/CommitEmailer/commit_emailer.py#L178-L179

But it is confusing to me because 1) it should match anything that isn't "master" and so all the commits to "main" should be filtered and 2) it should have fired on my PR...

@tstellar
Copy link
Collaborator

tstellar commented Nov 8, 2023

I discovered that the code in the repo does not match what is deployed, so I've update the repo now. I'm still not sure why it is failing though. I need a more recent commit to look at, because the webhook log only goes back so far.

@nikic
Copy link
Contributor Author

nikic commented Nov 30, 2023

It looks like this problem still exists, I keep getting "[𝘀𝗽𝗿] changes introduced through rebase" mails from @jroelofs.

@jroelofs
Copy link
Contributor

oh, hey, @AaronBallman was just asking me about those...

@jroelofs
Copy link
Contributor

@jroelofs
Copy link
Contributor

There is a branch detection here: https://github.com/llvm/llvm-admin/blob/main/AWS/Lambda/CommitEmailer/commit_emailer.py#L178-L179

But it is confusing to me because 1) it should match anything that isn't "master" and so all the commits to "main" should be filtered and 2) it should have fired on my PR...

The before/after here make sense in a world where there are only master, main, and release branches:

llvm/llvm-admin@a3b0398

Now that we have personal branches, maybe it ought to be something like (untested):

exclude_branches = ['users']
if any(x in event['ref'] for x in exclude_branches):
    break

@joker-eph
Copy link
Collaborator

I don't quite get it? The code is:


            exclude_branches = ['main', 'master']
            if not any(x in event['ref'] for x in exclude_branches):
                mail_to = LLVM_BRANCH_COMMITS_ADDRESS

My read is that is the branch is not main or master, the email address is switched to LLVM_BRANCH_COMMITS_ADDRESS = "llvm-branch-commits@lists.llvm.org" : so why are these commit sent to the other mailing list right now?

@nikic
Copy link
Contributor Author

nikic commented Nov 30, 2023

I think this is because main is contained in the branch name, e.g. users/bogner/spr/main.hlsl-add-helpers-to-simplify-hlsl-resource-type-declarations-nfc. We should be doing an exact match, not an in.

@jroelofs
Copy link
Contributor

I'm not sure how to test that ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Bugs about LLVM infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants