-
Notifications
You must be signed in to change notification settings - Fork 17
Establish Infrastructure #105
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
base: main
Are you sure you want to change the base?
Conversation
Amazing! On the file structure, since this repo is also meant to store files related to the project, can I suggest that we create a |
I was also wondering if you could break down the PR a bit more. I think there are changes here that are ready to be merged independently |
It seems feasible by writing a filter to the josh, though I haven't tried it yet. IMO it's OK to keep our
Yea makes sense. I'll split it up. |
CI wants some rustfmting |
env: | ||
TOOLS_BIN: "/tmp/smir/bin" | ||
# Note that the downloaded version is dated 2025-08-09. | ||
toolchain: nightly-2025-08-10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a toolchain toml file, as we'll update it frequently and that's easier to automate than editing a github workflow
Fine for now tho if you want to land this first and tune later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah makes sense. I'd like to update it later together with the automation pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that be a follow up PR?
This changes the rustc_public crate. Can you check that it still works inside the rust repo if synced back? |
The rustc compilation fails directly since we have a really strict msrv in our build.rs. Can we use the |
wait, is it possible to keep the files for the crates.io version in a separate directory? We can specify which Cargo.toml to use when building, and Cargo.toml itself can define the lib and build files. |
Yea. I think the plan was to overwrite the sources of the crates.io copy every time we make a breaking change? |
Yes the rustc version would be the base of a new major release |
uses: actions/checkout@v4 | ||
|
||
- name: Rust Toolchain | ||
uses: dtolnay/rust-toolchain@nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need nightly for format check?
env: | ||
TOOLS_BIN: "/tmp/smir/bin" | ||
# Note that the downloaded version is dated 2025-08-09. | ||
toolchain: nightly-2025-08-10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that be a follow up PR?
TOOLS_BIN: "/tmp/smir/bin" | ||
# Don't fail CI for now. See: https://github.com/rust-lang/project-stable-mir/issues/39 | ||
continue-on-error: true | ||
latest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, you should be able to create an action and reuse it for these two jobs. Not a big deal at this point, but if you have a follow up PR, that might be a nice change.
I also want to add a test for latest stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, you should be able to create an action and reuse it for these two jobs. Not a big deal at this point, but if you have a follow up PR, that might be a nice change.
Yes, that would be done as follow up
I also want to add a test for latest stable
ummm iirc the stable version can’t enable rustc_private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People would still need to set bootstrap env variable
Ok(()) | ||
} | ||
|
||
macro_rules! regexes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these regexes for? I thought ui_test handled this for us. @oli-obk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, e.g., it won't normalize 0x0000000000000000
to $HEX
, and the line and column info to :LL:CC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! Can you add a comment saying what patterns we are matching and why? Regex are always so painful to maintain. Thanks
harness = false | ||
|
||
[features] | ||
# tidy-alphabetical-start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove these directives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we don't have the tidy
tool in our repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this one. On the one hand, this crate will get synchronized with the rust repo, where they do enforce this. It will also be consistent with other rustc crates. On the other hand, leaving it on without guardrails can make sync a bit more painful.
I would suggest we leave it on and add a comment to folks changing Cargo.toml to ensure imports are in alphabetical order. If this becomes a hassle, we can remove it.
name = "rustc_public" | ||
version = "0.1.0-preview" | ||
authors = ["rustc_public team"] | ||
description = "Define compiler intermediate representation usable by external tools" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙂
BTW, sorry for the delay. Next time, do you mind creating separate commits for moving files and content changes. It makes it much easier to visualize the content changes. Otherwise, everything looks like new code. |
Oh sorry about that, thanks for the reminder though. I'll make sure to do that next time. |
I feel like maintaining two versions of |
Can we use a cargo feature to indicate that we are being built as part of the compiler? |
Yea but I think we'd still have to maintain two Cargo.toml. |
I actually think a config is probably the right way to go. We can set the config as part of the compiler bootstrap. Wouldn't we be able to keep the same code in that case? |
Sorry for the delay. I think the problem is that they don't share the same dependencies in the Cargo.toml, so if we sync things like But TBH now I feel OK with just maintaining two versions of Cargo.toml files and using a cargo feature to indicate where we're being built. And we could add a filter for the josh sync to exclude the Cargo.toml :) What do you think? |
If you feel comfortable implementing that, I think it would be maintainable and understandable, so I'm good with it |
Ok... let's start there. We can adjust as we go. Thanks! |
build
is implemented here. This PR implements./x build
,./x test [--bless]
,./x fmt [--check]
,./x githook install/uninstall
and./x clean
. Windows is not supported yet.compiletest
crate into a single file, making it possilbe to run test suites usingcargo test
.