Ryan Barth (Jun 27 2024 at 22:54): Ryan Barth (Jun 27 2024 at 22:54):Hey @Luke Boswell and @Anton I got a chance to look at 6808 today. I am trying to get my head around what you have done. So it looks like the new version of roc preprocess-host in the PR requires you to pass the --host --platform and --lib flags explicitly pointing to the appropriate files instead of being produced as part of the build process?
The failures I get in the test suite locally are all in the CLI tests. Looks like this exactly matches the existign failed CI run. Doesn't that make sense though? Many of the examples provide their own platforms and will have to be updated with the new preprocess-host command? They all seem to fail with a similar error message.
Taking a look at the first failed test expects_dev_and_test calls test_roc_app on crates/cli/tests/expects/expects.roc. Reading that file it looks like it uses a local implementation of the zig platform. Scanning through a few more of the failed tests reveals they also use local platform sources.
To fix this it seems like I could preprocess these local hosts as part of the test or use some prebuilt base. Preferences?
Luke Boswell (Jun 27 2024 at 23:07):I think those tests should be unrelated to this change. The preprocess-host is a subcommand for the cli. You run it like
roc preprocess-host /path/to/exe /path/to/platform/main.roc path/to/app.dylib
The CLI tests are using a release or local platform and I don't think any of them are using preprocess-host -- I'm not 100% on that.
Ryan Barth (Jun 27 2024 at 23:09):Hmm ok, yeah I have not seen preprocess-host in any I have looked at so far.
Luke Boswell (Jun 27 2024 at 23:10):I'm just having a look now
Ryan Barth (Jun 27 2024 at 23:10):All of these CLI tests pass for me on the main branch. I will try bringing that feature branch up to date with main.
Luke Boswell (Jun 28 2024 at 07:56):Great news. :tada:
@Ryan Barth and I were able to fix roc PR #6808 for Linux and MacOS, and test that it correctly generates the surgical linker files. We used the refactor-host basic-cli PR #194 to confirm this by uncommenting these lines in the build script.
Both of these PRs are now almost ready to merge. If we did that today we would have;
preprocess-host subcommand .rs host files out into crates.roc build script to generate the basic-cli binaries (for both legacy and surgical linkers)The only remaining issue is generating stub_dll_symbols for Windows -- which is currently left as a todo because I wasn't sure what we wanted to do with this.
error: cannot find macro `internal_error` in this scope
--> crates\cli\src/main.rs:174:17
|
174 | internal_error!("TODO populate stub_dll_symbols for Windows");
| ^^^^^^^^^^^^^^
Brendan Hansknecht 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.
Based on this comment, am I correct that we should remove the stub_dll_symbols from Windows and related failing tests?
Luke Boswell (Jun 28 2024 at 08:08):The basic-webserver #54 PR also refactors the host into crates and adds a build script.
I thought that would also be paired with the above pre-process API change and need to be coordinated. I was waiting for the API change to be ready so that we could also generate the surgical linker binaries for basic-webserver.
Ryan and I tested this today, and we found what looks like a surgical linker bug.
I can see that the current basic-webserver release does not have any surgical linker binaries, and assume this is because they can't be built with this bug.
Therefore, I don't think the basic-webserver #54 PR is not blocked on this change, and we can progress that independently.
Brendan Hansknecht (Jun 28 2024 at 14:59):Yeah, let's remove the stub lib command
Brendan Hansknecht (Jun 28 2024 at 15:00):Just make platform authors compile a simple app with build --lib
Luke Boswell (Jun 28 2024 at 20:52):@Brendan Hansknecht I'm talking about the internals not a subcommand. Do we need the sub lib symbols to make the surgical linker things for Windows?
Luke Boswell (Jun 28 2024 at 20:55):https://github.com/search?q=repo%3Aroc-lang%2Froc%20stub_dll_symbols&type=code
Brendan Hansknecht (Jun 29 2024 at 16:25):I think that is currently still needed, but is a hack
Brendan Hansknecht (Jun 29 2024 at 16:26):Essentially, we really should be using glue to tell us what host function actually get generated
Brendan Hansknecht (Jun 29 2024 at 16:26):Instead, we take some base function names and attempt to generate all possible wrapper names.
Luke Boswell (Jun 29 2024 at 20:21):I'm not sure how we should proceed with this PR then. :thinking:
Is there some way to defer that for a follow up? Maybe we lose surgical linking on Windows for now... but that ok because it's got a critical bug anyway?
Brendan Hansknecht (Jun 29 2024 at 20:22):Is there a reason that specific function can just be left alone for a follow up?
Brendan Hansknecht (Jun 29 2024 at 20:23):*can't
Luke Boswell (Jun 29 2024 at 20:23):It's killing CI on that PR
Luke Boswell (Jun 29 2024 at 20:23):Well, actually I havent tested removing the todo!.
Brendan Hansknecht (Jun 29 2024 at 20:24):Wait....what does the current windows surgical linker do instead....I must be missing soemthing
Luke Boswell (Jun 29 2024 at 20:24):But I'm leaning towards leaving it alone. It's just with this PR it's no longer wired up.
Luke Boswell (Jun 29 2024 at 20:36):It currently takes a .roc file and uses that to generate the stubbed lib symbols. These symbols are only used in the Windows preprocess host.
With this API change we are no longer providing a path to a .roc stubb app, but instead a dylib - the stubbed app dylib that that was used to link with and build the host.
Luke Boswell (Jun 29 2024 at 20:40):All this said, I think we can remove the dependency on an actual dylib and the stub symbols.
I think I looked into removing the dependency on the actual dylib, so all we should need to pass in is the host executable and the platform main. But I think I couldn't for some reason.
I'll try again and see.
Brendan Hansknecht (Jun 29 2024 at 20:49):Sorry, that comment only meant for the stub lib dylib.
Brendan Hansknecht (Jun 29 2024 at 20:49):Still probably want an app.roc or app dylib
Last updated: Jul 06 2025 at 12:14 UTC