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

core::compiler: remove Executor trait #11428

Closed
wants to merge 1 commit into from

Conversation

nrc
Copy link
Member

@nrc nrc commented Nov 27, 2022

What does this PR try to resolve?

This PR remove the Executor trait and thus simplifies some code. I added this trait for the RLS and it was never advertised as a publicly available API. The RLS is dead now and I think this API is not the best way to achieve its goals, therefore I think it should be removed.

How should we test and review this PR?

Change is trivial refactoring, so existing tests should be fine. Eyeball review should be fine.

Additional information

r? @arlosi

@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2022

Failed to set assignee to arlosi: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Hello Nick! I am happy that RLS retired gloriously and reducing code complexity sounds great to me! However, I am not sure i there are projects depend on Executor trait heavily. From my naive search result on GitHub, there are at least three projects depending on it:

I didn't look into them. Perhaps we can provide them alternatives (if those tools are still in maintenance).

@nrc
Copy link
Member Author

nrc commented Nov 27, 2022

The first project hasn't been touched for four years, but the other two seem to be maintained. I guess we need to keep supporting this API unless we can provide an alternative or there is really good reason not to.

@bors
Copy link
Contributor

bors commented Nov 29, 2022

☔ The latest upstream changes (presumably #11430) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo weihanglo added S-blocked and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2023
@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-blocked labels Apr 18, 2023
@weihanglo weihanglo marked this pull request as draft April 18, 2023 21:56
weihanglo referenced this pull request in not-fl3/cargo-quad-apk May 26, 2023
Now rust 1.50 is supported
@weihanglo
Copy link
Member

Just posted comments for those projects. I'll take over this PR if either they agree to this or no reply for a while.

@weihanglo
Copy link
Member

Okay. Got a reply from the author of cargo-c lu-zero/cargo-c#324 (comment). Given cargo-c is a relatively popular project, I don't think we can merge this pull request in any near future. I'll close it for now. Thank you.

@weihanglo weihanglo closed this May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants