Spu7Nix / SPWN-language

A language for Geometry Dash triggers
MIT License
1.05k stars 61 forks source link

Fixed compile issue on Mac, Linux, and Android #321

Closed Deltara3 closed 1 year ago

Deltara3 commented 1 year ago

Due to the live_editor member of the BuildSettings struct only compiling in on Windows, other platforms would simply error upon attempting to access it.

DexterHill0 commented 1 year ago

Can you confirm it won't compile on other platforms? Lines 40, 42 and 94 will only import the live editor structs if compiling for windows, so there should be no errors on other platforms. The --live-editor / -e argument is disabled on other platforms as it has no use.

zTags commented 1 year ago

i think the issue is in

            let is_live_editor = if cfg!(target_os = "windows") {
                settings.live_editor
            } else {
                false
            };

cause cfg!(target_os = "windows") will expand to false but won't remove the body of the if statement itself if you were to use cfg-if that'd probably fix it cause it will only expand to settings.live_editor if compiling on windows, instead of having it in an if statement like the current solution

Deltara3 commented 1 year ago

Can you confirm it won't compile on other platforms? Lines 40, 42 and 94 will only import the live editor structs if compiling for windows, so there should be no errors on other platforms. The --live-editor / -e argument is disabled on other platforms as it has no use.

I can confirm it wouldn't as I am a Linux user.

DexterHill0 commented 1 year ago

Can you confirm it won't compile on other platforms? Lines 40, 42 and 94 will only import the live editor structs if compiling for windows, so there should be no errors on other platforms. The --live-editor / -e argument is disabled on other platforms as it has no use.

I can confirm it wouldn't as I am a Linux user.

What is the error(s) you get? Is it just the one that would come from the code tags highlighted? I don't wanna remove the cfg attrs to enable the argument on other platforms seeing as it has no use atm. If you can could you fix the source of the error then I'll merge that?

Deltara3 commented 1 year ago

I was actually working on that before you commented lmao. I just added a commit that retains the intended functionality. Should work, but just in case it would be a good idea to test on Windows before merging. (Don't see why it wouldn't.)

DexterHill0 commented 1 year ago

Can you pull latest changes just cause the BasicError stuff is outdated, then I'll merge?

Deltara3 commented 1 year ago

Ah it did it again that's annoying, sure I'll do that.

Deltara3 commented 1 year ago

Alright, I've fixed that. Also error.rs didn't have the cfg attributes for Windows so I added that too. I think my fork may be a bit broken lmao.

DexterHill0 commented 1 year ago

Seems all good now will merge