Replace cli flag `--prebuilt-platform` with `--build-host` #6859

lukewilliamboswell · 2024-07-02T11:36:07Z

This PR is built on top of #6808, which should be merge first.

This PR makes progress towards decoupling roc from the host toolchains, by moving host rebuilding behind a compiler flag so it is no longer the default behaviour. It is still available where required using --build-host however will give a warning;

WARNING: platforms are responsible for building hosts, this flag will be removed when internal test platforms have a build script

I was originally working towards removing rebuilding in one go in #6696, however I realised this was a very large and difficult change due to the need to change all of the cli tests.

This PR achieves the same effect, effectively removing platform rebuilding from the cli for end users -- while keeping the cli tests as they are. This enables the cli tests to be adapted one by one instead of all at once. Once all of the tests have a build script, host rebuilding functionality can be entirely removed along with the associated dependencies.

Application authors who use a platform release from a URL package are not affected by this change. These users will continue to use a prebuilt platform.

Platform authors need to include a build script in their platform to build host binaries. This change will make running platform example less convenient if they are not using a URL release, as the platform binaries will need to be built first in a separate step. For example this workflow;

Build the platform binary roc build.roc using host toolchain e.g. cargo or zig
Run the example application roc examples/hello-world.roc

crates/compiler/roc_target/src/lib.rs

WIP WIP WIP ignore some cli tests that are going to move try fix for linux valgrind tests try a legacy host for valgrind fix roc_glue tests

lukewilliamboswell · 2024-07-09T04:43:01Z

crates/compiler/build/src/program.rs

    let platform_main_roc = match &loaded.entry_point {
        EntryPoint::Executable { platform_path, .. } => platform_path.to_path_buf(),
        _ => unreachable!(),
    };

-    // For example, if we're loading the platform from a URL, it's automatically prebuilt
-    // even if the --prebuilt-platform CLI flag wasn't set.
-    let is_platform_prebuilt = prebuilt_requested || loaded.uses_prebuilt_platform;


I should add a check here; if loaded.uses_prebuilt_platform is true and also build_host_requested then the platform is a URL release and we have also been instructed to rebuild the host with the flag --build-host. These two are incompatible because roc cannot build the host.

lukewilliamboswell · 2024-07-09T04:48:13Z

crates/compiler/roc_target/src/lib.rs

+
+    // file name for a stubbed app dynamic library file
+    pub fn stub_app_lib_file_name(&self) -> String {
+        format!("libapp.{}", self.dynamic_library_file_ext())


On Windows I think it may just be app.dll, on unix* systems we need the lib prepended

…uild-platform

Signed-off-by: Luke Boswell <[email protected]>

lukewilliamboswell · 2024-08-18T07:13:30Z

crates/compiler/build/src/program.rs

+        // Copy preprocessed host to executable location.
+        // The surgical linker will modify that copy in-place.
+        //std::fs::copy(&preprocessed_path, &output_exe_path).unwrap();


TODO - investigate why this is commented out.

crates/cli/tests/cli_run.rs

Anton-4 · 2024-09-03T14:42:52Z

crates/cli/tests/cli_run.rs

        }

        #[test]
        #[cfg_attr(windows, ignore)]
        fn nqueens() {
-            test_benchmark("nQueens.roc", &["6"], "4\n", UseValgrind::Yes)
+            test_benchmark("nQueens.roc", vec!["6"], "4\n", UseValgrind::Yes)


Why does this use vec now?

Anton-4 · 2024-09-03T14:50:03Z

crates/cli_utils/src/helpers.rs

+    /// Normalise the output for comparison in tests by replacing with a placeholder
+    fn normalize_for_tests(input: &str) -> String {
+        // normalise from windows line endings to unix line endings
+        let without_clrf = input.replace("\r\n", "\n");
+
+        // remove ANSI color codes
+        let without_ansi = roc_reporting::report::strip_colors(without_clrf.as_str());
+
+        // replace timings with a placeholder
+        let regex = Regex::new(r" in (\d+) ms\.").expect("Invalid regex pattern");
+        let replacement = " in <ignored for test> ms.";
+        let without_timings = regex.replace_all(&without_ansi, replacement);
+
+        // replace file paths with a placeholder
+        let filepath_regex =
+            Regex::new(r"\[([^:]+):(\d+)\]").expect("Invalid filepath regex pattern");
+        let filepath_replacement = "[<ignored for tests>:$2]";
+        let without_filepaths = filepath_regex.replace_all(&without_timings, filepath_replacement);
+
+        // replace error summary timings
+        let error_summary_regex =
+            Regex::new(r"(\d+) error(?:s)? and (\d+) warning(?:s)? found in \d+ ms")
+                .expect("Invalid error summary regex pattern");
+        let error_summary_replacement = "$1 error and $2 warning found in <ignored for test> ms";
+        error_summary_regex
+            .replace_all(&without_filepaths, error_summary_replacement)
+            .to_string()
+    }


Nice :) I don't think we had this before

Anton-4 · 2024-09-03T16:35:35Z

crates/cli/tests/cli_run.rs

-        )
+        let expected_ending = "Roc <3 Rust!\n🔨 Building host ...\n";
+        let runner = Run::new_roc()
+            .arg(CMD_RUN)


Assuming that run still tries to run a program even if there are errors, that is probably not what we want to use here...

lukewilliamboswell · 2024-09-04T12:08:26Z

TODO

ensure that roc without a subcommand behaves like roc dev does today and doesn't run with any errors
run cli tests without any subcommand.
implement ROC_EXPERIMENTAL_DEFER_ERRORS flag

see https://roc.zulipchat.com/#narrow/stream/395097-compiler-development/topic/which.20roc.20subcommand.20for.20roc_cli.20tests/near/467492272

Anton-4 · 2024-09-06T06:36:56Z

I'll get started on moving the build process out from under valgrind like we currently do on main

Anton-4 · 2024-09-06T15:21:58Z

I'm not a fan of this java style:

impl Run {
    pub fn new<S>(exe: S) -> Self
    where
        S: AsRef<OsStr>,
    {
        let exe: OsString = exe.as_ref().into();
        Self {
            args: vec![exe],
            stdin_vals: vec![],
            env: vec![],
            cwd: None,
            run_with_valgrind: false,
        }
    }

    pub fn new_roc() -> Self {
        Self::new(path_to_roc_binary())
    }

    pub fn command(&self) -> Command {
        let mut cmd = Command::new(&self.args[0]);
        for arg in self.args[1..].iter() {
            cmd.arg(arg);
        }
        for (k, v) in self.env.iter() {
            cmd.env(k, v);
        }
        cmd
    }

    pub fn valgrind_command(&self) -> (Command, NamedTempFile) {
        let mut cmd = Command::new("valgrind");

I can't easily see anymore which values a function actually uses, the old version on main was not great either but I am going to take a stab at a functional refactor.

Sidenote: Be careful with grouping things in utils files/folders, this can slow compilation because many crates can end up relying on a fat utils crate/file with many dependencies. Also make sure to put dependencies that are only required for testing under dev-dependencies in Cargo.toml

Anton-4 · 2024-09-06T18:11:30Z

This will take a while, btw I'm not getting rid of a lot, just mostly shuffling things around, feeling what makes the most sense and is least complex.

I'm also going to try removing all the valgrind xml handling code, we may not be using that anymore.

smores56 · 2024-09-08T01:43:32Z

I've reviewed everything except for build/src/program.rs. I think it all looks good except for two comments I left. I'll let you know once I've gotten to that file.

smores56 · 2024-09-08T01:48:28Z

@Anton-4 the new builder style is pretty popular in Rust because of a lack of optional or named parameters. If it had those, then we wouldn't need builders. What are you planning to do instead? I think something like this could work:

#[derive(Default)]
pub struct RunParams {
    args: Vec<OsString>,
    env: Vec<(String, String)>,
    stdin_vals: Vec<&'static str>,
    cwd: Option<OsString>,
    run_with_valgrind: bool,
}

pub fn run_command(cmd: Command, params: RunParams);

let out =
    run_command(
        ROC_CLI,
        RunParams {
            args: ["abc", "def"].into(),
            env: [("ROC_TEST", "1")].into(),
            ..Default::default(),
        }
    ).unwrap();

smores56 · 2024-09-06T04:43:26Z

crates/cli/tests/cli_run.rs

+                    .run();
+                out.assert_clean_success();
+            });
+        }

        fn test_benchmark(


I like that the above tests show all of the steps we're taking: build the host if necessary, run the test with these args, check the output. We don't seem to do that for the benchmark tests. Are you planning on making these two groups work the same way eventually, or is there a reason we have them working differently?

smores56 · 2024-09-06T05:11:17Z

crates/cli_utils/src/helpers.rs

 use std::path::PathBuf;
 use std::process::{Command, ExitStatus, Stdio};
 use std::sync::Mutex;
 use tempfile::NamedTempFile;

+lazy_static::lazy_static! {


nitpick: LazyLock has been stabilized, we should prefer that instead of an external library.

Anton-4 · 2024-09-09T10:24:09Z

What are you planning to do instead?

Right now, I just have two functions roc_run_default and roc_run. roc_run_default has less arguments, and roc_run has all of them. I'm going to try to finish my code today to see if that approach works.

I think something like this could work

That also seems like a nice alternative with low complexity, I'll keep it in mind.

Anton-4 · 2024-09-09T18:10:51Z

I'm going to try to finish my code today to see if that approach works.

Still need to do some refactoring of the glue tests and valgrind tests, targeting tomorrow now...

Anton-4 · 2024-09-10T16:55:25Z

So... looks like the Builder does make the best trade-offs after all 😄

Just functions does not work because we need to able to clone Command for that, which is not possible.
And the struct calls end up looking pretty messy:

let roc_cmd_out =
            run_roc_maybe_both_linkers(
                RocCmdParams {
                    sub_command: CMD_BUILD,
                    args: all_to_os_string([
                        roc_file_path.to_str().unwrap(),
                        BUILD_HOST_FLAG,
                        SUPPRESS_BUILD_HOST_WARNING_FLAG,
                        ]),
                    ..Default::default()
                },
                ON_LINUX
            );

Well, lessons learned, I should have asked more questions instead of dismissing it.

Anton-4 · 2024-09-10T16:57:30Z

I did make some nice improvements in other places though, I'm going to add those here.

Anton-4 · 2024-09-10T17:47:53Z

Need to make sure I don't forget this:

Ensure cli tests run with both linkers on linux

lukewilliamboswell commented Jul 2, 2024

View reviewed changes

crates/compiler/roc_target/src/lib.rs Show resolved Hide resolved

lukewilliamboswell force-pushed the rebuild-platform branch from cd13762 to 8383fd6 Compare July 9, 2024 00:02

lukewilliamboswell added 3 commits July 9, 2024 13:48

WIP

dc0902b

WIP WIP WIP ignore some cli tests that are going to move try fix for linux valgrind tests try a legacy host for valgrind fix roc_glue tests

fix: building valgrind surgical host

753cfb4

replace stray strings with impl on Target

85447ec

lukewilliamboswell force-pushed the rebuild-platform branch from 4521f92 to 85447ec Compare July 9, 2024 04:39

lukewilliamboswell commented Jul 9, 2024

View reviewed changes

lukewilliamboswell and others added 8 commits July 9, 2024 14:52

minor fixes

96c559e

replace gen-stub-lib in nix CI tests

4ccbc41

remove gen_stub_lib

287d00d

clippy fix

f515311

add --build-host for benchmark tests

bd68787

checkpoint: refactor prebuilt host artifacts

61a5a0a

Merge remote-tracking branch 'remotes/luke/rebuild-platform' into reb…

d747cc9

…uild-platform

checkpoint: it compiles

dfa3254

This was referenced Jul 15, 2024

Refactor host into crate roc-lang/basic-cli#194

Merged

Compiler Panics when --prebuild-platform is omitted. #6815

Open

checkpoint: Run struct refactor for helpers

bc29782

lukewilliamboswell changed the title ~~WIP replace cli flag --prebuilt-platform with --build-host~~ Replace cli flag --prebuilt-platform with --build-host Aug 14, 2024

lukewilliamboswell force-pushed the rebuild-platform branch from 5f8ff89 to 61d6b5f Compare August 14, 2024 22:36

Fix ownership issues with runners

89d63f2

Signed-off-by: Luke Boswell <[email protected]>

smores56 force-pushed the rebuild-platform branch from 61d6b5f to 89d63f2 Compare August 15, 2024 01:28

lukewilliamboswell and others added 7 commits August 15, 2024 11:50

WIP get things compiling... hacks everywhere beware

3662c90

Get tests working

4ec213a

Merge branch 'main' into rebuild-platform

550bade

Merge branch 'main' into rebuild-platform

49f5ea8

Fix formatting and clippy issues

28ceb0e

restore one test

2928969

WIP more cli_run tests passing

6276cc7

lukewilliamboswell commented Aug 18, 2024

View reviewed changes

lukewilliamboswell added 2 commits September 3, 2024 20:35

Merge remote-tracking branch 'remote/main' into rebuild-platform

012a2fc

fixup

a11f51a

Anton-4 reviewed Sep 3, 2024

View reviewed changes

crates/cli/tests/cli_run.rs Outdated Show resolved Hide resolved

Anton-4 reviewed Sep 3, 2024

View reviewed changes

Anton-4 added 2 commits September 3, 2024 18:12

clippy fix

ec8f6e8

misc improvements

251e1ed

Anton-4 reviewed Sep 3, 2024

View reviewed changes

lukewilliamboswell added 7 commits September 4, 2024 20:56

migrate cli test combine-tasks.roc off basic-cli

ef87653

remove basic-cli tests from roc_cli

c6a67c9

migrate inspect-logging.roc into cli tests and use effects platform

5dfcdae

add test for module_params pass_task.roc

73846ed

remove unneeded basic-cli reference in formatting test

beb2dcb

remove unneded basic-cli reference in test_reporting

bb57c4a

remove last usage of basic-cli platform from the tests

dc138b8

lukewilliamboswell mentioned this pull request Sep 5, 2024

DO NOT MERGE Upgrade llvm 18 zig 13 #6921

Open

lukewilliamboswell marked this pull request as ready for review September 5, 2024 03:12

smores56 reviewed Sep 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace cli flag `--prebuilt-platform` with `--build-host` #6859

Replace cli flag `--prebuilt-platform` with `--build-host` #6859

lukewilliamboswell commented Jul 2, 2024 •

edited

Loading

lukewilliamboswell Jul 9, 2024 •

edited

Loading

lukewilliamboswell Jul 9, 2024

lukewilliamboswell Aug 18, 2024

Anton-4 Sep 3, 2024

Anton-4 Sep 3, 2024

Anton-4 Sep 3, 2024

lukewilliamboswell commented Sep 4, 2024

Anton-4 commented Sep 6, 2024

Anton-4 commented Sep 6, 2024

Anton-4 commented Sep 6, 2024

smores56 commented Sep 8, 2024

smores56 commented Sep 8, 2024

smores56 Sep 6, 2024

smores56 Sep 6, 2024

Anton-4 commented Sep 9, 2024

Anton-4 commented Sep 9, 2024

Anton-4 commented Sep 10, 2024 •

edited

Loading

Anton-4 commented Sep 10, 2024

Anton-4 commented Sep 10, 2024

Replace cli flag --prebuilt-platform with --build-host #6859

Are you sure you want to change the base?

Replace cli flag --prebuilt-platform with --build-host #6859

Conversation

lukewilliamboswell commented Jul 2, 2024 • edited Loading

lukewilliamboswell Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

lukewilliamboswell Jul 9, 2024

Choose a reason for hiding this comment

lukewilliamboswell Aug 18, 2024

Choose a reason for hiding this comment

Anton-4 Sep 3, 2024

Choose a reason for hiding this comment

Anton-4 Sep 3, 2024

Choose a reason for hiding this comment

Anton-4 Sep 3, 2024

Choose a reason for hiding this comment

lukewilliamboswell commented Sep 4, 2024

Anton-4 commented Sep 6, 2024

Anton-4 commented Sep 6, 2024

Anton-4 commented Sep 6, 2024

smores56 commented Sep 8, 2024

smores56 commented Sep 8, 2024

smores56 Sep 6, 2024

Choose a reason for hiding this comment

smores56 Sep 6, 2024

Choose a reason for hiding this comment

Anton-4 commented Sep 9, 2024

Anton-4 commented Sep 9, 2024

Anton-4 commented Sep 10, 2024 • edited Loading

Anton-4 commented Sep 10, 2024

Anton-4 commented Sep 10, 2024

Replace cli flag `--prebuilt-platform` with `--build-host` #6859

Replace cli flag `--prebuilt-platform` with `--build-host` #6859

lukewilliamboswell commented Jul 2, 2024 •

edited

Loading

lukewilliamboswell Jul 9, 2024 •

edited

Loading

Anton-4 commented Sep 10, 2024 •

edited

Loading