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

Add MacOS Utility executable #20647

Merged
merged 1 commit into from
Feb 18, 2023
Merged

Add MacOS Utility executable #20647

merged 1 commit into from
Feb 18, 2023

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Jan 30, 2023

I was made aware that that OpenRA.Utility.exe was removed from macOS builds as part of upgrading to dotnet 6, but I'm unable to find the PR. This change was less than ideal as now in order to be able to use the utility, macOS users are forced to download and compile source.

Compiled utility Utility.zip
You use it by cd'ing to game files MacOS folder and running ./Utility [yaml-command]

As this was the first time I've programmed in C / Objective C this most likely is low quality code and needs a refactor. Ping @pchote

@PunkPun PunkPun added this to the Next release milestone Jan 30, 2023
@PunkPun PunkPun requested a review from pchote January 30, 2023 18:30
@penev92
Copy link
Member

penev92 commented Feb 3, 2023

OpenRA.Utility.exe was removed from macOS builds as part of upgrading to dotnet 6, but I'm unable to find the PR.

I'm going to guess it was #18955

@abcdefg30
Copy link
Member

Adding the label since we need to figure out what to do with the duplication/if we can reuse the existing launcher.

@pchote
Copy link
Member

pchote commented Feb 3, 2023

I'd expect this to be directly invoking the hostfxr instead of using an NSTask (which is there to provide the crash dialog!).

@PunkPun
Copy link
Member Author

PunkPun commented Feb 8, 2023

Invoking hostfxr directly now. I've resolved part of the duplication issue and the utility can now be properly terminated with ctrl + c

packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
@PunkPun PunkPun force-pushed the utility branch 2 times, most recently from 014967d to dc7c31b Compare February 12, 2023 10:11
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Some more style comments (not yet tested):

packaging/macos/utility.m Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
@PunkPun PunkPun force-pushed the utility branch 3 times, most recently from 90cab6d to 27f1d92 Compare February 12, 2023 13:03
packaging/macos/utility.m Outdated Show resolved Hide resolved
for (int i = argc; i > 0; i--)
argv[i + 1] = argv[i - 1];

argv[0] = (char*)[hostPath UTF8String];
Copy link
Member

Choose a reason for hiding this comment

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

These in general wouldn't be safe (UTF8String returns a pointer to an internal buffer that will be overwritten and/or released, which is why the types don't match without the cast), but since we're immediately calling into _mono_main I think it will be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be better to just rewrite it in regular C so these conversions wouldn't be necessary. But I won't be doing that.

packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
@PunkPun PunkPun force-pushed the utility branch 2 times, most recently from ca42c18 to 9b13cb0 Compare February 12, 2023 22:25
penev92
penev92 previously approved these changes Feb 18, 2023
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

I'm just going to put this here for the record and if noone complains or reviews I'll just merge.

👍

packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
packaging/macos/utility.m Outdated Show resolved Hide resolved
@PunkPun
Copy link
Member Author

PunkPun commented Feb 18, 2023

prep build

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

ok

@penev92 penev92 merged commit 6997c44 into OpenRA:bleed Feb 18, 2023
@PunkPun PunkPun deleted the utility branch February 18, 2023 15:15
@penev92
Copy link
Member

penev92 commented Feb 18, 2023

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants