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.

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

Remove platform rebuilding and generate_stub_lib #6696

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

lukewilliamboswell
Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell commented Apr 30, 2024

zulip discussion

This PR

  • removes the generate_stub_lib
  • removes rebuilding host functionality from the roc cli
  • modifies roc preprocess-host

Roc platform hosts' are responsible for building using their native toolchain so they provide the binaries for legacy and surgical linking. The roc cli no longer has any dependency on a host toolchain.

The API for pre-processing the host has changed to the following:

$ cargo run -- preprocess-host --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.31s
     Running `target/debug/roc preprocess-host --help`
Runs the surgical linker pre-processor to generate `.rh` and `.rm` files.

Usage: roc preprocess-host [OPTIONS] <host> <platform> <lib>

Arguments:
  <host>      Path to the host executable where the app was linked dynamically
  <platform>  Path to the platform/main.roc file
  <lib>       Path to a stubbed app dynamic library (e.g. roc build --lib app.roc)

Options:
      --target <target>  Choose a different target [default: macos-arm64] [possible values: linux-x32, linux-x64, linux-arm64, macos-x64, macos-arm64, windows-x32, windows-x64, windows-arm64, wasm32]
      --verbose          Print detailed information while pre-processing host
  -h, --help             Print help

Copy link
Contributor

@bhansconnect bhansconnect left a comment

Choose a reason for hiding this comment

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

I think we should really change the ux here as part of this change.

I am also really not a fan of requiring platforms to include some sort of special libapp.roc and dynhost just to enable correctly linking. These are the types of confusing implicit deps that make it hard to use.

Like, probably can just be:

roc build --lib examples/simple.roc
cargo build
roc preprocess-host --host target/build/host --platform platform/main.roc

If we want/need to still depend on the dylib, lets make it explicit instead of locking in on libapp.so that happens to be in the platform directory.

roc build --lib examples/simple.roc
cargo build
roc preprocess-host --host target/build/host --platform platform/main.roc --lib examples/simple.dylib

Fundamentally, the libapp and dynhost naming should die. It is just a weird constraint that will confuse users, especially if it is implicit like this.


I also am questioning if it would be possible to just use an example as the actual input. The example once parsed knows where the platform is. If the example is example/simple.roc, the dylib should be example/simple.dylib. I guess that still leave an open question about host binary though.


All this said, I think we can remove the dependency on an actual dylib and the stub symbols. So I think we should remove both of those. The only piece of information we actually need is what shared library the host thinks it linked to such that we can remove that dependency during preprocessing.

So I guess, I currently lean most towards this api if it works:

roc build --lib examples/simple.roc
cargo build
roc preprocess-host --host target/build/host --app examples/simple.roc

crates/cli/src/main.rs Outdated Show resolved Hide resolved
@lukewilliamboswell lukewilliamboswell changed the title Remove generate_stub_lib from roc cli Remove generate_stub_lib and platform rebuilding May 1, 2024
@lukewilliamboswell lukewilliamboswell changed the title Remove generate_stub_lib and platform rebuilding WIP Remove platform rebuilding and generate_stub_lib May 1, 2024
@lukewilliamboswell
Copy link
Collaborator Author

Current progress for cargo test -p roc_cli

failures:
    cli_run::hello_gui
    cli_run::inspect_gui
    cli_run::known_type_error
    cli_run::platform_switching_main
    cli_run::platform_switching_rust
    cli_run::platform_switching_swift
    cli_run::platform_switching_zig
    cli_run::ruby_interop
    cli_run::static_site_gen
    cli_run::swift_ui
    cli_run::terminal_ui_tea

test result: FAILED. 46 passed; 11 failed; 1 ignored; 0 measured; 0 filtered out; finished in 30.70s

error: test failed, to rerun pass `-p roc_cli --test cli_run`

Copy link
Contributor

Choose a reason for hiding this comment

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

Task is used very heavily here, I'd be in favor of going with import pf.Task as T.

Comment on lines +40 to +54
arch =
Cmd.new "uname"
|> Cmd.arg "-m"
|> Cmd.output
|> Task.map .stdout
|> Task.map Help.archFromStr
|> Task.mapErr! \err -> ErrGettingNativeArch (Inspect.toStr err)

os =
Cmd.new "uname"
|> Cmd.arg "-s"
|> Cmd.output
|> Task.map .stdout
|> Task.map Help.osFromStr
|> Task.mapErr! \err -> ErrGettingNativeOS (Inspect.toStr err)
Copy link
Contributor

@Anton-4 Anton-4 May 6, 2024

Choose a reason for hiding this comment

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

We should make functions for these in basic-cli, that will clean things up roc-lang/basic-cli#203.

@Anton-4
Copy link
Contributor

Anton-4 commented May 6, 2024

This looks good to me in general :)

Comment on lines 8 to 17
pub extern "C" fn main() {
// let bounds = roc::Bounds {
// width: 1900.0,
// height: 1000.0,
// };

// gui::run_event_loop("RocOut!", bounds).expect("Error running event loop");
println!("TODO FIX PLATFORM IMPLEMENTATION");
std::process::exit(0);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Anton-4 -- do you think we should try and keep both breakout and the gui platform? They aren't in a good state currently. I have made changes so they at least compile and are usable (though breakout I have basically broken entirely) for CI purposes. What is our plan/thoughts longer term?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's take them out, I don't think they've been a net positive overall.

Comment on lines +1 to +12
interface Help
exposes [
RocTarget,
Arch,
Os,
archFromStr,
osFromStr,
rocTarget,
prebuiltBinaryName,
dynamicLibraryExtension,
]
imports []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO - move to a better location than in examples

@lukewilliamboswell lukewilliamboswell changed the title WIP Remove platform rebuilding and generate_stub_lib Remove platform rebuilding and generate_stub_lib May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants