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

ruff: initial integration #11471

Closed
wants to merge 1 commit into from
Closed

ruff: initial integration #11471

wants to merge 1 commit into from

Conversation

manunio
Copy link
Contributor

@manunio manunio commented Jan 9, 2024

No description provided.

Copy link

github-actions bot commented Jan 9, 2024

manunio is integrating a new project:
- Main repo: https://github.com/astral-sh/ruff
- Criticality score: 0.75288

@addisoncrump
Copy link
Contributor

addisoncrump commented Jan 10, 2024

@manunio, the authors have mentioned that they do not yet wish to integrate Ruff as the existing fuzzers and this one are finding lots of issues that are not desirable to fix. I have previously communicated with them separately about OSS-Fuzz integration and they are not for it presently. 🙂

As an aside, the relevant issue tracking this is: astral-sh/ruff#4972 (incomplete list of issues found by fuzzers that need to be resolved before integration is considered)
And the fuzzers are here: astral-sh/ruff#4822

@qarmin should also be involved in the initial integration, as he has contributed by far the most fuzzing-discovered issues.

CC: @charliermarsh (project lead) for comment here as well.

@manunio
Copy link
Contributor Author

manunio commented Jan 10, 2024

@manunio, the authors have mentioned that they do not yet wish to integrate Ruff as the fuzzers are finding lots of issues that are not desirable to fix. I asked about this as well 🙂

@addisoncrump Thanks for the update, will keep this pr here as a draft, in-case they are interested at later time.

@addisoncrump
Copy link
Contributor

addisoncrump commented Jan 10, 2024

Very well.

@qarmin and I have both spent several months of effort on fuzzing, categorising issues, and assisting with resolution. Put bluntly, it would be inappropriate to integrate our work without consulting us, especially if your goal (as I suspect) is to apply for rewards. If the Ruff team ever decides to integrate, the three of us need to have a conversation separately about this.

In the future, you should really discuss this (privately or publicly) with the relevant code owners. This is not how I wanted to broach this conversation.

As an aside, the listed vendor ccs should also include:

@manunio
Copy link
Contributor Author

manunio commented Jan 10, 2024

Very well.

@qarmin and I have both spent several months of effort on fuzzing, categorising issues, and assisting with resolution. Put bluntly, it would be inappropriate to integrate our work without consulting us, especially if your goal (as I suspect) is to apply for rewards. If the Ruff team ever decides to integrate, the three of us need to have a conversation separately about this.

In the future, you should really discuss this privately with the relevant code owners. This is not how I wanted to broach this conversation.

As an aside, the listed vendor ccs should also include:

@addisoncrump you are free to go ahead with integration, I have not asked the ruff team yet before your comment, infact I can't go ahead with integration unless I have explicit permission from ruff authors. I was going to ask ruff team today about what they wish to do here, i'll close this pr and ruff's pr and please go ahead with your own pr :)

@manunio manunio closed this Jan 10, 2024
@addisoncrump
Copy link
Contributor

addisoncrump commented Jan 10, 2024

That's not what I was suggesting... this PR (as well as your fuzzer which you just closed separately) is fine, we just need to have a conversation before this happens. I am not trying to dissuade you from integrating, I am pointing out that a) I have already spoken to the developers and wanted to share what they have previously told me, and b) it was a little inconsiderate to open this without speaking to any of the other folks working on fuzzing this target already. That doesn't mean you need to close this issue, or the fuzzer upstream which has already also found bugs. We just need to collaborate going forward.

@manunio
Copy link
Contributor Author

manunio commented Jan 10, 2024

That's not what I was suggesting... this PR (as well as your fuzzer which you just closed separately) is fine, we just need to have a conversation before this happens. I am not trying to dissuade you from integrating, I am pointing out that a) I have already spoken to the developers and wanted to share what they have previously told me, and b) it was a little inconsiderate to open this without speaking to any of the other folks working on fuzzing this target already. That doesn't mean you need to close this issue, or the fuzzer upstream which has already also found bugs. We just need to collaborate going forward.

@addisoncrump I think it's better for me to close this pr(as you are right, i can't integrate your work) and keep will upstream pr open, sorry for the confusion and not making things clear earlier, please feel free to open a pr here, when you receive permission upstream :)

@manunio manunio deleted the ruff branch January 10, 2024 12:39
@addisoncrump
Copy link
Contributor

Okay. If the Ruff team ever decides to integrate with OSS-Fuzz, let's open a PR again.

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

Successfully merging this pull request may close these issues.

2 participants