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

[mono] Implement System.Console for iOS #33827

Merged
merged 13 commits into from
Mar 21, 2020
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 19, 2020

Implements #33667

@EgorBo
Copy link
Member Author

EgorBo commented Mar 20, 2020

@marek-safar addressed.

@marek-safar marek-safar added the os-ios Apple iOS label Mar 20, 2020
{
// Write in chunks of max 4096 characters; older versions of iOS seems to have a bug where NSLog may hang with long strings (!).
// https://github.com/xamarin/maccore/issues/1014
const char* utf8 = [msg UTF8String];
Copy link
Member Author

Choose a reason for hiding this comment

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

According to the issue it reproduces even on iOS 11 which is higher than the minimal version we support as far as I understand.

Copy link
Member

Choose a reason for hiding this comment

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

This is Objective C? Do we have any other use of Objective C in System.Native? Is it required to build this for iOS?

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's probably possible to avoid it but does it matter? It's iOS only and we most likely will need to bring more iOS specific pieces here for other APIs, most likely for globalization stuff

Copy link
Member

Choose a reason for hiding this comment

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

It's iOS only

How should I interpret that in conjunction with Marek's comments about the interop .cs file being moved to a general mac folder?

does it matter?

I don't know. I don't have a lot of experience with Objective C and whatever toolset requirements it might or might not place on the repo.

Copy link
Member

Choose a reason for hiding this comment

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

How should I interpret that in conjunction with Marek's comments about the interop .cs file being moved to a general mac folder?

Replied on the other thread, it should be some generic apple folder, not mac.

Objective C and whatever toolset requirements it might or might not place on the repo.

No dependencies, it's part of the default XCode toolchain you need on Mac for everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I guess next will be Environment.GetFolderPath https://github.com/xamarin/xamarin-macios/blob/3f0985ecac82b00aada1a41a464b15d1f58b6744/runtime/xamarin-support.m#L173-L182 so objc will be unavoidable (to call the native API).

@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

You put this under src/Interop/OSX. Is that correct? Or should there be an iOS-specific folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly there is no general mac folder where this would be better placed (https://developer.apple.com/documentation/foundation/1395275-nslog) or is the intention to copy the interop to System.Native implementation for all OSes?

Copy link
Member

Choose a reason for hiding this comment

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

Sadly there is no general mac folder where this would be better placed

Meaning this API is available on any mac, whether OSX or iOS or watch or whatever? We can certainly create a folder for such APIs, assuming we're also making sure that the export it's targeting from System.Native is built on all such platforms. Or if we're only building in the export for iOS, then an iOS-specific folder would make more sense. Presumably in the fullness of time we'll find some APIs that are specific to iOS and so we'll want an iOS folder anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd move it to iOS honestly (just like in my previous commit) we are not going to use this API on OSX, only iOS/tvOS/watchOS which all are basically the same OS almost 🙂

Copy link
Contributor

@marek-safar marek-safar Mar 20, 2020

Choose a reason for hiding this comment

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

It's possible that we find some API which will be ios only but at the level, we operate it's most likely that PAL for ios, tvos, watchos, catalyst and some parts macOS will be identical

We could use ios name for modern apple or whatever is the better name but it might be confusing for the contributors

Copy link
Member

Choose a reason for hiding this comment

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

I'd use something generic like apple for the pieces that work across mac/ios/tvos/watchos.

Doesn't need to be done here though since there are a lot of cases where we'll need to rename the Interop/OSX folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

so are we ok with OSX then?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. It does not make sense to try to redo the directory structure in this PR.


void SystemNative_Log (uint8_t* buffer, int32_t length)
{
NSString *msg = [[NSString alloc] initWithBytes: buffer length: length encoding: NSUTF16LittleEndianStringEncoding];
Copy link
Member

Choose a reason for hiding this comment

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

How does error handling work in Objective C? Does this throw exceptions? What is the usual way to deal with it?

Copy link
Member Author

@EgorBo EgorBo Mar 20, 2020

Choose a reason for hiding this comment

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

initWithBytes returns nil for invalid data, then all messages ([obj message]) are no-op or return 0/nil/null. The only thing - strlen won't be ok with NULL, let me fix it.
NSLog is also fine with nil.

@EgorBo EgorBo merged commit 9c23058 into dotnet:master Mar 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants