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

Switch to in-tree rustc dependencies with a cfg flag #15616

Merged
merged 1 commit into from Sep 19, 2023

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Sep 15, 2023

We can use this flag to detect and prevent breakages in rustc CI. (see #14846 and #15569)

The IN_RUSTC_REPOSITORY is just a placeholder. Is there any existing cfg flag that rustc CI sets?

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2023
@HKalbasi HKalbasi force-pushed the rustc-deps branch 2 times, most recently from 8cb5a84 to 11470a6 Compare September 15, 2023 10:26
@lnicola
Copy link
Member

lnicola commented Sep 15, 2023

Is there any existing cfg flag that rustc CI sets?

It enables the in-rust-tree feature, IIUC.

@HKalbasi
Copy link
Member Author

This will probably make syncs harder, but I think it does worth it. Feel free to revert if it ended up breaking something or caused too much trouble.

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 19, 2023

📌 Commit f4704bc has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 19, 2023

⌛ Testing commit f4704bc with merge 22b18b9...

@bors
Copy link
Collaborator

bors commented Sep 19, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 22b18b9 to master...

@bors bors merged commit 22b18b9 into rust-lang:master Sep 19, 2023
10 checks passed
@lnicola
Copy link
Member

lnicola commented Sep 19, 2023

This will probably make syncs harder.

I hope not 😬.

@bors bors mentioned this pull request Sep 19, 2023
@Veykril
Copy link
Member

Veykril commented Sep 19, 2023

Not a fan of the implications this has for our dependency graph. This is probably fine right now but won't scale in the future.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

While this is probably a solution, I have to say that I really don't like these changes 😅

@@ -19,6 +19,7 @@

#![warn(rust_2018_idioms, unused_lifetimes, semicolon_in_expressions_from_macros)]
#![allow(rustdoc::private_intra_doc_links)]
#![cfg_attr(feature = "in-rust-tree", feature(rustc_private))]
Copy link
Member

Choose a reason for hiding this comment

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

why is this required?

Comment on lines +94 to +102
in-rust-tree = [
"sysroot-abi",
"ide/in-rust-tree",
"syntax/in-rust-tree",
"parser/in-rust-tree",
"rustc-dependencies/in-rust-tree",
"hir-def/in-rust-tree",
"hir-ty/in-rust-tree",
]
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of this either, this will ultimately end up with mentioning almost all of our crates at some point

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too, maybe a non cargo cfg is a better fit here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it'd be better if we could make it so that this doesn't thread through all the cargo files if possible

@@ -8,6 +8,7 @@
//! actually true.

#![warn(rust_2018_idioms, unused_lifetimes, semicolon_in_expressions_from_macros)]
#![cfg_attr(feature = "in-rust-tree", feature(rustc_private))]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This feature is needed even if you import the rustc private crates indirectly, which I think is unnecessarily too aggressive. Is there any way to depend on the crates in the rustc workspace without this feature? Clippy and Miri use this, but they are fine with nightly and feature flags.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, since we might mention the items from those crates now. Well that's unfortunate but not much we can do in that case. Would be nice to add a comment regarding that then.

@@ -19,6 +19,7 @@
//! [RFC]: <https://github.com/rust-lang/rfcs/pull/2256>
//! [Swift]: <https://github.com/apple/swift/blob/13d593df6f359d0cb2fc81cfaac273297c539455/lib/Syntax/README.md>

#![cfg_attr(feature = "in-rust-tree", feature(rustc_private))]
Copy link
Member

Choose a reason for hiding this comment

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

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants