feat: setup bootstrap and ci #3

Wybxc · 2024-09-19T12:26:54Z

This pull request sets up the bootstrap process and CI for the C codegen backend.

jieyouxu

I left some minor nits. I also have a question for this backend: is there a reason why you need to reinvent a bootstrap instead of forking the rust repo then adding additional logic as necessary?

EDIT: to clarify, it's entirely possible that you have very good reasons or run into difficult limitations for doing so. I'm asking because using Yet Another Bootstrap makes (1) upstreaming the backend very very difficult and (2) makes it more difficult for other rustc contributors who wants to make changes to this backend because they have to deal with the intricacies of Yet Another Build System.

bootstrap/.gitignore

bootstrap/src/clean.rs

bootstrap/src/fmt.rs

bootstrap/src/test.rs

Wybxc · 2024-09-20T12:06:30Z

I left some minor nits. I also have a question for this backend: is there a reason why you need to reinvent a bootstrap instead of forking the rust repo then adding additional logic as necessary?我留下了一些小虱子。我对这个后端还有一个问题：是否有理由需要重新发明引导程序，而不是分叉rust存储库，然后根据需要添加额外的逻辑？

EDIT: to clarify, it's entirely possible that you have very good reasons or run into difficult limitations for doing so. I'm asking because using Yet Another Bootstrap makes (1) upstreaming the backend very very difficult and (2) makes it more difficult for other rustc contributors who wants to make changes to this backend because they have to deal with the intricacies of Yet Another Build System.编辑：澄清一下，您完全有可能有很好的理由或遇到困难的限制。我这么问是因为使用 Yet Another Bootstrap 会（1）使后端上游变得非常非常困难，并且（2）使其他想要更改此后端的 rustc 贡献者变得更加困难，因为他们必须处理 Yet Another 的复杂性构建系统。

In projects such as rustc_codegen_cranelift or rust_codegen_gcc, there exists a component referred to as the build_system, which serves a similar function to what is termed "bootstrap" in this context. It is important to distinguish this from the bootstrap process of the Rust compiler itself. In this instance, we assume the presence of an operational Rust compiler, and we merely encapsulate some command-line invocations to rustc, making rustc_codegen_c the default code generation option.

Wybxc · 2024-09-20T12:07:53Z

Refer to #2 (comment)

tgross35

I think this looks pretty good overall. Please just make sure absolute paths are used everywhere, otherwise it looks like a lot of things will break when running from a different directory.

Aside from that, a few other comments but I don't see any big issues.

bootstrap/.gitignore

tgross35 · 2024-09-20T15:03:40Z

.github/workflows/main.yaml

-        run: |
-          rustup update nightly --no-self-update
-          rustup default nightly
-          rustup component add clippy
+        run: rustup show


You should be able to use rustup toolchain install to make sure the version from the toolchain file is used, since rust-lang/rustup#3983

This feature is not yet available in the release version of rustup. Perhaps we can wait for the next release of rustup before switching the approach here.

tgross35 · 2024-09-20T15:06:28Z

bootstrap/src/clean.rs

+
+impl CleanCommand {
+    pub fn run(&self, manifest: &Manifest) {
+        let _ = std::fs::remove_dir_all("crates/target");


This should be an absolute path presumably based on location information in Manifest, so running from a different location doesn't remove the wrong thing.

tgross35 · 2024-09-20T15:08:13Z

bootstrap/src/fmt.rs

+                .args(["--manifest-path", "bootstrap/Cargo.toml"]),
+        );
+        self.perform(
+            Command::new("cargo")
+                .arg("fmt")
+                .args(["--manifest-path", "crates/Cargo.toml"])
+                .arg("--all"),
+        );
+        for file in glob("example/**/*.rs").unwrap() {
+            self.perform(
+                Command::new("rustfmt")
+                    .args(["--edition", "2021"])
+                    .arg(file.unwrap()),
+            );
+        }
+        for file in glob("tests/**/*.rs").unwrap() {


This should use absolute paths too.

tgross35 · 2024-09-20T15:09:38Z

bootstrap/src/fmt.rs

+        }
+    }
+
+    pub fn perform(&self, command: &mut Command) {


Nit: perform and run should probably get different names since it isn't very obvious what the difference is. Or ideally, just put this into a trait so all steps are required to use the same interface.

Nonblocker though, this can be changed later.

tgross35 · 2024-09-20T15:10:56Z

bootstrap/src/main.rs

+    /// debug mode
+    #[arg(short, long)]
+    pub debug: bool,


This could probably be called verbose based on how it is use. debug sounds like it would be the opposite of release.

tgross35 · 2024-09-20T15:12:44Z

bootstrap/src/manifest.rs

+    pub out_dir: PathBuf,
+}
+
+impl Manifest {


Double check that absolute paths are used in this block

tgross35 · 2024-09-20T15:20:47Z

bootstrap/src/test.rs

+            "FileCheck",
+            "FileCheck-14",
+            "FileCheck-15",
+            "FileCheck-16",
+            "FileCheck-17",
+            "FileCheck-18",


Nit: you probably want to reverse the order so newer filecheck versions get preferred?

One additional question: Should we prefer FileCheck or FileCheck-18?

If you can be specific, then pinning it to a specific FileCheck version seems nicer for reproducibility (and bumps to which FileCheck version is used then becomes explicit)

Wybxc · 2024-09-22T09:32:42Z

I think this looks pretty good overall. Please just make sure absolute paths are used everywhere, otherwise it looks like a lot of things will break when running from a different directory.

Aside from that, a few other comments but I don't see any big issues.

The use cases for bootstrap are typically highly specific to the project structure, making execution in alternative directories impractical. Additionally, the brevity of the ./y command serves as a clear reminder of the current working directory.

As far as I am aware, rustc_codegen_gcc also utilizes relative paths:
https://github.com/rust-lang/rustc_codegen_gcc/blob/3018d9dc8fc2fe19a0e7366166ceb6829d1b273d/build_system/src/clean.rs#L72
https://github.com/rust-lang/rustc_codegen_gcc/blob/3018d9dc8fc2fe19a0e7366166ceb6829d1b273d/build_system/src/main.rs#L16

feat: setup bootstrap and ci

c19d4b7

Wybxc mentioned this pull request Sep 19, 2024

Minimal codegen support: numbers and function paramters #2

Open

jieyouxu reviewed Sep 19, 2024

View reviewed changes

Wybxc added 4 commits September 20, 2024 20:11

chore: remove useless Path::new

719d2aa

chore: endline

28e2411

fix: panic on clean

9ab2c93

fix: status check of fmt

0842a73

tgross35 reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: setup bootstrap and ci #3

feat: setup bootstrap and ci #3

Wybxc commented Sep 19, 2024

jieyouxu left a comment •

edited

Loading

Wybxc commented Sep 20, 2024

Wybxc commented Sep 20, 2024

tgross35 left a comment

tgross35 Sep 20, 2024

Wybxc Sep 22, 2024

tgross35 Sep 20, 2024

tgross35 Sep 20, 2024

tgross35 Sep 20, 2024

tgross35 Sep 20, 2024

tgross35 Sep 20, 2024

tgross35 Sep 20, 2024

Wybxc Sep 22, 2024

jieyouxu Sep 22, 2024

Wybxc commented Sep 22, 2024

feat: setup bootstrap and ci #3

Are you sure you want to change the base?

feat: setup bootstrap and ci #3

Conversation

Wybxc commented Sep 19, 2024

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

Wybxc commented Sep 20, 2024

Wybxc commented Sep 20, 2024

tgross35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wybxc commented Sep 22, 2024

jieyouxu left a comment •

edited

Loading