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.

Sign up for GitHub

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

Jump to bottom

internal: sync from downstream #16317

Merged
merged 11 commits into from
Jan 9, 2024
Merged

internal: sync from downstream #16317

merged 11 commits into from
Jan 9, 2024

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jan 9, 2024

No description provided.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2024
@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2024

📌 Commit f52f2f9 has been approved by lnicola

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 9, 2024

⌛ Testing commit f52f2f9 with merge f4eef1c...

bors added a commit that referenced this pull request Jan 9, 2024
@bors
Auto merge of #16317 - lnicola:sync-from-rust, r=lnicola
f4eef1c 
internal: sync from downstream
@bors
Copy link
Contributor

bors commented Jan 9, 2024

💔 Test failed - checks-actions

@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024

@bors r-

lnicola
lnicola commented Jan 9, 2024
View reviewed changes
.github/workflows/ci.yaml Outdated
@@ -48,6 +48,7 @@ jobs:
CC: deny_c
RUST_CHANNEL: "${{ needs.changes.outputs.proc_macros == 'true' && 'nightly' || 'stable' }}"
USE_SYSROOT_ABI: "${{ needs.changes.outputs.proc_macros == 'true' && '--features sysroot-abi' || '' }}"
RUSTUP_RUSTC_DEV: "${{ needs.changes.outputs.proc_macros == 'true' && 'rustc-dev' || '' }}"
Copy link
Member Author

Choose a reason for hiding this comment

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

We want this, right?

Copy link
Member

Choose a reason for hiding this comment

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

No we shouldn't need this 🤔 our CI should not rely on the rustc crates

Veykril
Veykril reviewed Jan 9, 2024
View reviewed changes
crates/proc-macro-srv/src/lib.rs
#![warn(rust_2018_idioms, unused_lifetimes)]
#![allow(unreachable_pub, internal_features)]

extern crate proc_macro;
extern crate rustc_driver as _;
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a cfg #[cfg(feature = "in-rust-tree")]

Veykril
Veykril reviewed Jan 9, 2024
View reviewed changes
crates/proc-macro-srv/src/lib.rs Outdated
@@ -11,11 +11,12 @@
//! rustc rather than `unstable`. (Although in general ABI compatibility is still an issue)…

#![cfg(any(feature = "sysroot-abi", rust_analyzer))]
#![feature(proc_macro_internals, proc_macro_diagnostic, proc_macro_span)]
#![feature(proc_macro_internals, proc_macro_diagnostic, proc_macro_span, 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.

This feature needs to be gated #[cfg_attr(feature = "in-rust-tree", feature(rustc_private)]

lnicola
lnicola commented Jan 9, 2024
View reviewed changes
crates/proc-macro-srv/src/lib.rs
@@ -11,11 +11,14 @@
//! rustc rather than `unstable`. (Although in general ABI compatibility is still an issue)…

#![cfg(any(feature = "sysroot-abi", rust_analyzer))]
#![cfg_attr(feature = "in-rust-tree", feature(rustc_private))]
Copy link
Member Author

Choose a reason for hiding this comment

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

Like this, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Thanks for doing the syncs as always, especially since they seem to have been more trouble the past weeks 😅

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

CI failure is due to a new lint on nightly fwiw, since this touches the proc-macro server source CI runs the nightly toolchain. You can probably just pick the changes from this file here https://github.com/rust-lang/rust-analyzer/pull/16309/files#diff-0449230bb140c3330091dc8be2f8b5d227e6875704429de5116f428fcbe7b532

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

🤨 I can't repro that, and nothing should be causing that here, might be spurious let's see
@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2024

📌 Commit b0b2b1c has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 9, 2024

⌛ Testing commit b0b2b1c with merge de290b8...

bors added a commit that referenced this pull request Jan 9, 2024
@bors
Auto merge of #16317 - lnicola:sync-from-rust, r=Veykril
de290b8 
internal: sync from downstream
@bors
Copy link
Contributor

bors commented Jan 9, 2024

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Hm, feel free to just cfg that test out for now (since ignoring is rejected by CI as well), I'll look into what's going on there later. Probably related to the dylib linking changes in the sysroot, fairly sure the error there is incorrect and its something else.

@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024

I can reproduce the failure using RUN_SLOW_TESTS=1 cargo test -p rust-analyzer --test slow-tests --features sysroot-abi resolve_proc_macro.

lnicola
lnicola commented Jan 9, 2024
View reviewed changes
crates/rust-analyzer/tests/slow-tests/main.rs
@@ -835,7 +835,7 @@ fn main() {
#[cfg(any(feature = "sysroot-abi", rust_analyzer))]
fn resolve_proc_macro() {
use expect_test::expect;
if skip_slow_tests() {
if skip_slow_tests() || true {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added an early exit here.

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Ugh right, I forgot to set the slow test var, repros for me as well then, that's good. Will take a look at that in a bit.

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Fairly certain the cause is #16307 (which didn't run the test)

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2024

📌 Commit 4413aeb has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 9, 2024

⌛ Testing commit 4413aeb with merge f5f7dda...

@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024 •
edited
Loading

Thanks for doing the syncs as always, especially since they seem to have been more trouble the past weeks

Months, you mean 😄. Anyway, it's the least I can do.

Fairly certain the cause is #16307 (which didn't run the test)

Yeah, reverting that fixes the failure :-(.

@bors
Copy link
Contributor

bors commented Jan 9, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f5f7dda to master...

@bors bors merged commit f5f7dda into rust-lang:master Jan 9, 2024
10 checks passed
@lnicola lnicola deleted the sync-from-rust branch January 9, 2024 08:48
@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Ye okay I know why, now that we no longer save the document in the vfs we can't filter out set_file_content calls that don't change the document value, and something seems to report changing the file content multiple times hence the failure.

@bors bors mentioned this pull request Jan 9, 2024
Merged
@lnicola
Copy link
Member Author

lnicola commented Jan 9, 2024 

2024-01-09T08:54:56.688606Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/bar/src/lib.rs)
2024-01-09T08:54:56.688821Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/foo/src/main.rs)
2024-01-09T08:54:56.689274Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/bar/Cargo.toml)
2024-01-09T08:54:56.689285Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/bar/src/lib.rs)
2024-01-09T08:54:56.689294Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/foo/Cargo.toml)
2024-01-09T08:54:56.689300Z  INFO vfs: set_file_contents(/tmp/testdir/87771_0/foo/src/main.rs)

CC #14730?

@Veykril
Copy link
Member

Veykril commented Jan 9, 2024

Yes, that pretty much (and another smaller issue).

This was referenced May 9, 2024
Closed
Closed
Closed
Closed
Merged
This was referenced May 28, 2024
Merged
Closed
Closed
Closed
Merged
@philips-software-forest-releaser philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024
Merged
This was referenced Jun 11, 2024
Merged
Merged
@philips-software-forest-releaser philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lnicola @bors @Veykril @rustbot