Allow disabling gui at compile time

[austenadler]

Allows users to disable the GUI at compile time by using --no-default-features

[benjamin051000]

I did not do code review, but nice work. I can test this in two days when I'm home.

We definitely should discuss how this might work with releases... having two executables, one with gui, one without, is kinda weird. Maybe this shouldn't be a compile feature... I don't know. Maybe the gui should live in its own repo. 🤷‍♂️ things to discuss I guess

[benjamin051000]

Oh, this also needs CI testing. We need to ensure it compiles with and without gui. I assume that shouldn't be too hard to add into the github workflow matrix thing.

[benjamin051000]

Resolves #159

[amir16yp]

if this is merged i can submit a PR that will add CLI-only version to releases with the workflows

[louis-e]

Sounds very good to me, I'll look into this asap! :)

[louis-e]

A few thoughts from my side, I like this implementation and I think it makes sense to do it in this way. Sadly there seems to be no way to make this line, which suppresses the command line, optional for a single release:

arnis/src/main.rs

Line 1 in a92c5bb

#![cfg_attr(not(debug_assertions), windows_subsystem = "windows")]

That's why I'm okay with a seperate CLI only release asset, as long as it's clearly distinguishable via the asset name (e.g. "arnis-cli.exe").
Also I somehow can't get this to run in CLI only mode, can you confirm that this is the correct way to execute it?

[austenadler]

Sadly there seems to be no way to make this line, which suppresses the command line, optional for a single release

Also I somehow can't get this to run in CLI only mode, can you confirm that this is the correct way to execute it?

Hmm I am not on Windows and I didn't try it there. I will check it out today and see why it's failing for windows builds. It might be the windows_subsystem line that's preventing an actual error from being printed?

[austenadler]

There are a few points here.

1: Your coordinates aren't generating any data, which is why it exited unsuccessfully. The output should have been:

[1/5] Fetching data...
Error! No data available in this region.
error: process didn't exit successfully: `target\release\arnis.exe --path=C:\Users\user\Downloads\superflat --bbox=48.139631,11.564054,48.145014,11.574440` (exit code: 1)

It works if you use a different set of coordinates (for example: cargo r -r -- "--path=C:\Users\user\Downloads\superflat" --bbox=-80.849226,34.117578,-80.841286,34.123032

2: Windows does not send anything to stdout/stderr if you use windows_subsystem = "windows" (by default). So you were not seeing that error message in Windows, but it would show up for linux. According to this and this, you can reattach to the parent console at runtime.

If you double click the .exe, you get the GUI. If you run .\arnis.exe, you get the GUI. If you specify --path=... --bbox=..., you get stdout. It's the best of both worlds.

3: According to the FreeConsole and AttachConsole documentation, you can safely free if not already attached, and I don't see any warnings about using these functions, so they should be fine to use even though they have to be in an unsafe block.

I pushed the fix to this. Everything should work now; please review again.

[louis-e]

Thanks a lot for your great work and sorry for catching up with it a bit late!
I've now reviewed and tested everything, this works perfectly! Thank you for your effort :)

[benjamin051000]

Woohoo! cargo build --no-default-features only has 528 deps to build, cargo build --release has 768!