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

Adding diesel to the cargetest suite #79599

Closed
wants to merge 1 commit into from

Conversation

weiznich
Copy link
Contributor

@weiznich weiznich commented Dec 1, 2020

This was proposed in #79459 and #79560.
Diesel stresses the trait system implementation quite a lot and there
have been various regressions regarding that in the past.

This was proposed in rust-lang#79459 and rust-lang#79560.
Diesel stresses the trait system implementation quite a lot and there
have been various regressions regarding that in the past.
@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2020
@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 1, 2020
Comment on lines +70 to +73
// Test the embeded sqlite variant of diesel
// This does not require any dependency to be present,
// sqlite will be compiled as part of the build process
features: Some(&["sqlite", "libsqlite3-sys/bundled"]),
Copy link
Member

Choose a reason for hiding this comment

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

How long does this take to compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A clean build takes ~1min on my not really powerful laptop and maybe ~1 minutes to run all tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds fine, thanks :)

@jyn514
Copy link
Member

jyn514 commented Dec 1, 2020

At some point it might also be interesting to add diesel to rustc-perf, but that doesn't necessarily have to happen now.

@jyn514 jyn514 added the A-traits Area: Trait system label Dec 1, 2020
@weiznich
Copy link
Contributor Author

weiznich commented Dec 1, 2020

At some point it might also be interesting to add diesel to rustc-perf, but that doesn't necessarily have to happen now.

That was already proposed here. I will try to have a look there as soon I've find some time. Diesel should definitively be an interesting workload there as it does spend most compile time inside of rustc and not in llvm. (At least that was the case last time I've looked at why does take that much time to compile).

@Mark-Simulacrum
Copy link
Member

I don't think we should be expanding the set of crates in the cargotest suite until we have a T-compiler/T-infra FCP'd policy about how additions should get reviewed (e.g., who approves? how?). So I'm going to close this for now, but I'd love to see such a policy written; the first step would be to draft something reasonable and file an MCP to discuss that I think.

@weiznich
Copy link
Contributor Author

weiznich commented Dec 9, 2020

@Mark-Simulacrum Who is the correct person to get in touch with to discuss creating such a policy? As already mentioned there where multiple diesel related issues in the last weeks + some more in the last year so this seems to be a good test case + diesel is also used as port of the official rust infrastructure (crates.io)

@jyn514
Copy link
Member

jyn514 commented Dec 9, 2020

@weiznich you can open an MCP here: https://github.com/rust-lang/compiler-team/issues/new/choose

but in general I think @Mark-Simulacrum 's point is that it isn't clear who to get in contact with and he's not comfortable making the decision unilaterally.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2021

Well then lets talk about it at the team meeting.

Nominating for discussion, in terms of the one-off situation of this particular test addition. (The more general policy can also be discussed, but I don't want that general discussion to distract from making a decision here.)

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2021

reopened as PR #81507, so I'll nominate that for discussion.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2021
…Mark-Simulacrum

Adding diesel to the cargotest suite

As discussed in rust-lang#79560 (comment) this adds diesel to the compilers test suite. This is basically a reopened version of rust-lang#79599, but now with the backing of the compiler team.

r? `@pnkfelix`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc A-traits Area: Trait system S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants