-
Notifications
You must be signed in to change notification settings - Fork 514
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
Added support for Windows builds of Xamarin.Mac #77
Conversation
Hi @joj, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
@joj, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Build failure |
// This is also an input | ||
[Output] | ||
public string SdkVersion { | ||
get; set; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style, move the get; set; to the same line as done in SessionId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with all the formatting comments, but this is a copy paste from the original and I didn't want to change it. See here: https://github.com/xamarin/xamarin-macios/pull/77/files#diff-2e650c9beeab2092efcab1aea606b5e6L19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the file moves (only) in a separate commits, then git should properly detect that and not show that many additions/deletions.
Then in a separate commit you can change things (in this case I see you've changed the name of the class, but I have no idea if you've changed anything else in the file, which seriously complicates reviewing the PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really a move, though. The original files where turned into bases (in a new Core project). The signature of the originals is unchanged, by them inheriting the bases. The only changes in the code of the bases is to add a SessionId property and the naming. My problem with doing it in separate commits is that I will potentially have a commit that will not build, and that breaks Minimum Unit of Cherry-Picking. None of the code in the bases changed, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this would not build:
- Create a project for a new assembly (Xamarin.Mac.Tasks.Core)
- Move all the files you want to move there, without changing any contents.
- Reference Xamarin.Mac.Tasks.Core from where the corresponding files were previously used.
Afaict that should still work, because all you did was move code to a new assembly.
Then the next step would be to make the changes you need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work.
Get Outlook for mobile
On Thu, May 26, 2016 at 2:35 AM -0700, "Rolf Bjarne Kvinge" [email protected] wrote:
In msbuild/Xamarin.Mac.Tasks.Core/Tasks/DetectSdkLocations.cs:
+using Xamarin.MacDev.Tasks;
+using Xamarin.MacDev;
+
+namespace Xamarin.Mac.Tasks
+{
- public class DetectSdkLocationsTaskBase : Task
- {
#region Inputs
public string SessionId { get; set; }
// This is also an input
[Output]
public string SdkVersion {
get; set;
I'm not sure why this would not build:
Create a project for a new assembly (Xamarin.Mac.Tasks.Core)
Move all the files you want to move there, without changing any contents.
Reference Xamarin.Mac.Tasks.Core from where the corresponding files were previously used.
Afaict that should still work, because all you did was move code to a new assembly.
Then the next step would be to make the changes you need.
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
Build success |
1 similar comment
Build success |
I think you'll also have to add the name of the new assembly to the |
@joj 👍 is the goal here is to get Xamarin.Mac (XM45|MacMobile) libaries compiling on Windows as something officially possible? If so awesome. Right now it's possible by compiling |
Closing PR as it now has conflicts and needs to be redone into a few commits anyway. |
@ghuntley baby steps; don't get too excited :) |
* Fix unrecognized extension build warning for credential providers * Bump Xamarin.MacDev. New commits in xamarin/Xamarin.MacDev: * xamarin/Xamarin.MacDev@af50d97 Add AutoFill CredentialProvider NSExtensionPoint support (xamarin#75) (xamarin#77) Diff: https://github.com/xamarin/Xamarin.MacDev/compare/7e9075cab0b959b739e840cc76250665b32efb6f..af50d97218b9c35cbe25bb68b9a3def5dcc13bd6 * Add IDE deployment target for credential provider Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
* Fix unrecognized extension build warning for credential providers * Bump Xamarin.MacDev. New commits in xamarin/Xamarin.MacDev: * xamarin/Xamarin.MacDev@af50d97 Add AutoFill CredentialProvider NSExtensionPoint support (#75) (#77) Diff: https://github.com/xamarin/Xamarin.MacDev/compare/7e9075cab0b959b739e840cc76250665b32efb6f..af50d97218b9c35cbe25bb68b9a3def5dcc13bd6 * Add IDE deployment target for credential provider Co-authored-by: Rolf Bjarne Kvinge <[email protected]> Co-authored-by: kiddailey <[email protected]>
This is the first part of support for Xamarin.Mac for Windows. It moves stuff we need in the windows tasks to a Mac.Core project so we can inherit from our own Mac.Tasks. It also adds SessionId to all the tasks in the targets for synchronization. Some packaging is missing (including the new dll in the package and modifying build so the dll will not be built when Mac is disabled in the build). I spoke with @jstedfast about these changes and he will help there. Also @jstedfast, comments are welcome :)