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

Bumping syntax-ap crates fails due to reduced visiblities #3903

Closed
josephlr opened this issue Nov 4, 2019 · 12 comments
Closed

Bumping syntax-ap crates fails due to reduced visiblities #3903

josephlr opened this issue Nov 4, 2019 · 12 comments
Assignees
@calebcartwright
Labels
blocked Blocked on rustc, an RFC, etc. help wanted

Comments

@josephlr
Copy link

josephlr commented Nov 4, 2019

I'm trying to fix rust-osdev/uefi-rs#104 (comment). Rustfmt needs a new engough syntax to support "efiapi". I tired to do what #3870 did and bump rustc-ap-syntax to version 626.

This caused a very large number of compiler errors. Some of them are caused by small changes like: rust-lang/rust@742ec4b, which are easy enough to fix. However, others errors are caused by changes like rust-lang/rust@c189565 that make a bunch of libsyntax's functionality private.

What should I do here? Should the libsyntax commits be reverted in the main rust-lang/rust repo?

CC: @Centril
@calebcartwright
Copy link
Member

This looks like it may be tricky. Do you know the first/earliest version of rustc-ap that supports efiapi?

IIRC rustfmt 1.4.9 is using 606 and the latest on master has been bumped to 610, but the visibility changes in 626 appear very problematic

@josephlr josephlr mentioned this issue Nov 4, 2019
Merged
@topecongiro
Copy link
Contributor

@josephlr Thank you for the report. I am aware of the visibility issue, and was planning to fix the issue once he refactoring thing has settled. We need to re-export everything we need in the libsyntax, adding comments to make sure they won't become private or removed in the future.

@topecongiro
Copy link
Contributor

cc rust-lang/rust#65324.

@topecongiro topecongiro added blocked Blocked on rustc, an RFC, etc. help wanted labels Nov 5, 2019
@mati865
Copy link
Contributor

mati865 commented Dec 4, 2019

@topecongiro IIUC this will block updating currently broken RLS.
If I'm correct this should be prioritised.

@mati865 mati865 mentioned this issue Dec 4, 2019
Closed
@est31
Copy link
Member

est31 commented Dec 5, 2019 •
edited
Loading

@josephlr @mati865 if it helps you, the latest version that doesn't have rust-lang/rust@c189565 yet is 615, and the latest version that doesn't have rust-lang/rust@742ec4b either is 612.

efiapi was merged much later in rust-lang/rust@2dd4e73 (earliest rustc-ap-syntax version is 626)

@topecongiro
Copy link
Contributor

@mati865 I do not think this is blocking the broken RLS as AFAIK RLS does not directly depend on rustc-ap-* crates.

@topecongiro
Copy link
Contributor

For anyone interested in tackling this issue, here are the instructions:

  1. Fork & clone this repo.
  2. Edit Cargo.toml and bump the version of rustc-ap-* crates to the latest version.
  3. Run cargo check. You will see TONS of compiler errors. Fix them if possible. For unfixable ones, we need to update the rustc source code.
  4. Fork & clone https://github.com/rust-lang/rust.
  5. Update libsyntax so that rustfmt can access structs/functions and methods that it depends on. Add a comment on each one of them so that they won't get hidden in future development (e.g., // Keep this public for external tools like rustfmt.).
  6. Send a PR to the rustc repo.
  7. Wait for the PR to get merged & wait for the rustc-ap-* crates to be published. Note that regularly rustc-ap-* crates are published once a week (https://crates.io/crates/rustc-ap-syntax).
  8. Come back to the rustfmt repo, bump the version of rustc-ap-* crates and see if everything works. If not, go back to 5 😞

@calebcartwright calebcartwright mentioned this issue Dec 20, 2019
Merged
@topecongiro topecongiro mentioned this issue Dec 20, 2019
Closed
@topecongiro
Copy link
Contributor

This is blocked by alexcrichton/rustc-auto-publish#14. cc @calebcartwright.

@calebcartwright
Copy link
Member

@rchaser53 are you working on this one too? (asking based on rust-lang/rust#67653).

I've got a good chunk of the changes made locally, mostly waiting to see how the new librustc_parse crate will become available.

If you're further along though I'm happy to stop and let you take it, just want to make sure we aren't duplicating efforts!

@rchaser53
Copy link
Contributor

Oh sorry😅
I have just started implementation, so I will stop it.

@calebcartwright calebcartwright mentioned this issue Jan 11, 2020
Merged
Centril added a commit to Centril/rust that referenced this issue Jan 12, 2020
@Centril
Rollup merge of rust-lang#68135 - calebcartwright:rustc-parse-visibil…
92ed032 
…ities, r=Centril

restore some rustc_parse visibilities for rustfmt

In rust-lang@c189565 some visibilities were reduced on the parse mod (which now resides in the rustc_parse crate) as part of some refactoring and splitting up of libsyntax. However, rustfmt needs access to a few of those items that are no longer visible.

This restores the visibility on those items rustfmt depends on.

rust-lang/rustfmt#3903 (comment)
rust-lang/rustfmt#4009

cc @topecongiro
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 12, 2020
@Dylan-DPC
Rollup merge of rust-lang#68135 - calebcartwright:rustc-parse-visibil…
e7e8435 
…ities, r=Centril

restore some rustc_parse visibilities for rustfmt

In rust-lang@c189565 some visibilities were reduced on the parse mod (which now resides in the rustc_parse crate) as part of some refactoring and splitting up of libsyntax. However, rustfmt needs access to a few of those items that are no longer visible.

This restores the visibility on those items rustfmt depends on.

rust-lang/rustfmt#3903 (comment)
rust-lang/rustfmt#4009

cc @topecongiro
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 13, 2020
@JohnTitor
Rollup merge of rust-lang#68135 - calebcartwright:rustc-parse-visibil…
ca806cf 
…ities, r=Centril

restore some rustc_parse visibilities for rustfmt

In rust-lang@c189565 some visibilities were reduced on the parse mod (which now resides in the rustc_parse crate) as part of some refactoring and splitting up of libsyntax. However, rustfmt needs access to a few of those items that are no longer visible.

This restores the visibility on those items rustfmt depends on.

rust-lang/rustfmt#3903 (comment)
rust-lang/rustfmt#4009

cc @topecongiro
@calebcartwright calebcartwright mentioned this issue Feb 1, 2020
Closed
@calebcartwright calebcartwright mentioned this issue Feb 9, 2020
Merged
@calebcartwright
Copy link
Member

@topecongiro - this can be closed with #4043 being merged

@calebcartwright
Copy link
Member

Closed via #4043

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@calebcartwright calebcartwright

Labels
blocked Blocked on rustc, an RFC, etc. help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mati865 @josephlr @est31 @rchaser53 @calebcartwright @topecongiro