Skip to content

Enable mono build on OSX#560

Merged
nulltoken merged 2 commits intolibgit2:vNextfrom
ben:osx-mono
Nov 11, 2013
Merged

Enable mono build on OSX#560
nulltoken merged 2 commits intolibgit2:vNextfrom
ben:osx-mono

Conversation

@ben
Copy link
Member

@ben ben commented Nov 10, 2013

This reworks the build.libgit2sharp.sh script to allow tests to run with Mono on OS X. Tested under Mono 3.2.4 on OS X Mavericks, and my 32-bit Ubuntu VM.

Fixes #557.

Copy link
Member

Choose a reason for hiding this comment

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

This -m32 looks like a workaround for an OSX issue, where IIRC the mono packages ship a 32-bit binary. On your average Linux distro (or other Unix) on an amd64 box, mono is built for the "native" architecture, and thus expects a a 64-bit .so file.

Even that aside, not many people will have the cross-compilation/multiarch support installed (as you don't often need it). Travis is such an example.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @carlosmn here, we shouldn't be trying to force anything. Can we be fancy on Mac and build fat binaries?

if [ `uname -s` = "Darwin" ]; then
    CFLAGS=-arch x86 -arch amd64
    LDFLAGS=-bundle
fi

If you want to be really crazy you can add -arch ppc on Mac OS < 10.8 and -arch ppc64 on Mac OS 10.5 only. But that would be really crazy.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Now THAT is crazy! :)

@nulltoken
Copy link
Member

/cc @dragan

@ben
Copy link
Member Author

ben commented Nov 10, 2013

Yay for CMake! Looks like Travis is happy, and OS X builds fine now. There are still a couple of failing tests, though:

    /Users/ben/src/libgit2sharp/CI-build.msbuild: error : LibGit2Sharp.Tests.ShadowCopyFixture.CanProbeForNativeBinariesFromAShadowCopiedAssembly: Assert.True() Failure
    /Users/ben/src/libgit2sharp/CI-build.msbuild: error :   at LibGit2Sharp.Tests.ShadowCopyFixture.CanProbeForNativeBinariesFromAShadowCopiedAssembly () [0x00000] in <filename unknown>:0
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
    /Users/ben/src/libgit2sharp/CI-build.msbuild: error : LibGit2Sharp.Tests.BranchFixture.CanCreateBranch(name: "Ångström"): Assert.NotNull() Failure
    /Users/ben/src/libgit2sharp/CI-build.msbuild: error :   at LibGit2Sharp.Tests.BranchFixture.CanCreateBranch (System.String name) [0x00000] in <filename unknown>:0
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0

I'll look into either fixing or skipping these.

@ben
Copy link
Member Author

ben commented Nov 11, 2013

Heh.

diff --git a/LibGit2Sharp.Tests/BranchFixture.cs b/LibGit2Sharp.Tests/BranchFixture.cs
index a6fcc30..4921879 100644
--- a/LibGit2Sharp.Tests/BranchFixture.cs
+++ b/LibGit2Sharp.Tests/BranchFixture.cs
@@ -30,7 +31,12 @@ public void CanCreateBranch(string name)
                 Assert.Equal("refs/heads/" + name, newBranch.CanonicalName);
                 Assert.NotNull(newBranch.Tip);
                 Assert.Equal(committish, newBranch.Tip.Sha);
-                Assert.NotNull(repo.Branches.SingleOrDefault(p => p.Name == name));
+                Assert.NotNull(repo.Branches.SingleOrDefault(p => {
+                    if (p.Name.EndsWith("m")) {
+                        Assert.False(true, "(" + p.Name + " == " + name + ") = " + (p.Name == name));
+                    }
+                    return p.Name == name;
+                }));

                 AssertRefLogEntry(repo, newBranch.CanonicalName,
                                   newBranch.Tip.Id,

Output:

LibGit2Sharp.Tests.BranchFixture.CanCreateBranch(name: "Ångström") [FAIL]
   (Ångström == Ångström) = False

1 ben bens-mbp_ _src_libgit2sharp zsh -1

I think this may be a precomposition thing. Should we just skip this test on OS X for now?

@nulltoken
Copy link
Member

I think this may be a precomposition thing.

/cc @arrbee

Should we just skip this test on OS X for now?

Let's do this.

@nulltoken
Copy link
Member

LibGit2Sharp.Tests.BranchFixture.CanCreateBranch(name: "Ångström") [FAIL]
  (Ångström == Ångström) = False

@ben Just for troubleshooting purpose, could you please output the bytes that those strings are made of?

@ben
Copy link
Member Author

ben commented Nov 11, 2013

p.Name: 41-00-0A-03-6E-00-67-00-73-00-74-00-72-00-6F-00-08-03-6D-00
name:   C5-00-6E-00-67-00-73-00-74-00-72-00-F6-00-6D-00

Yup, p.Name has been decomposed.

@ben
Copy link
Member Author

ben commented Nov 11, 2013

Okay, I've fixed one more failure, and it's green. Here's what my output looks like now:

xUnit.net console test runner (32-bit .NET 2.0.50727.1433)
Copyright (C) 2013 Outercurve Foundation.

xunit.dll:     Version 1.9.0.1566
Test assembly: /Users/ben/src/libgit2sharp/LibGit2Sharp.Tests/bin/Debug/LibGit2Sharp.Tests.dll

LibGit2Sharp.Tests.StageFixture.CanStageANewFileWithAFullPath(ignorecase: True) [SKIP]
   Skipping 'ignorecase = true' test due to ignorecase issue in libgit2.

LibGit2Sharp.Tests.CloneFixture.CanCloneWithCredentials [SKIP]
   Populate Constants.PrivateRepo* to run this test

LibGit2Sharp.Tests.DiffTreeToTreeFixture.CanDetectTheRenamingOfAModifiedFile [SKIP]
   Not implemented in libgit2 yet.

LibGit2Sharp.Tests.FetchFixture.CanFetchIntoAnEmptyRepositoryWithCredentials [SKIP]
   Populate Constants.PrivateRepo* to run this test

LibGit2Sharp.Tests.BlobFixture.CanReadBlobFilteredStream(autocrlf: "true", expectedContent: "hey there
") [SKIP]
   Non-Windows does not support core.autocrlf = true

LibGit2Sharp.Tests.BlobFixture.CanGetBlobAsFilteredText(autocrlf: "true", expectedText: "hey there
") [SKIP]
   Non-Windows does not support core.autocrlf = true

LibGit2Sharp.Tests.BranchFixture.CanCreateBranch(name: "Ångström") [SKIP]
   UTF-8 decomposition on OSX throws a wrench in this

930 total, 0 failed, 7 skipped, took 57.065 seconds

@ethomson
Copy link
Member

Canonically, even. We could String.Normalize this instead of skipping it.

@ben
Copy link
Member Author

ben commented Nov 11, 2013

You're right, that's better.

@dahlbyk
Copy link
Member

dahlbyk commented Nov 11, 2013

Do we have any tests that exercise remote branches with special characters?

@ben
Copy link
Member Author

ben commented Nov 11, 2013

Should we? We already cover remotes, and once the branch is local, we cover that too.

@nulltoken
Copy link
Member

I'm not familiar with string.Normalize(). But rather than "fixing" the test to make it pass, shouldn't we do this at the EncodingMarshaler level?

@dahlbyk
Copy link
Member

dahlbyk commented Nov 11, 2013

No idea, just thinking out loud. I guess OSX would reencode the tracking branch file names just as it's messing with the normal branch file names?

@ethomson
Copy link
Member

I don't think so. The string is being marshalled correctly and represents
the correct filename. The test is expecting a filename that cannot
possibly exist on a Mac.

On Mon, Nov 11, 2013 at 10:43 AM, Keith Dahlby notifications@github.comwrote:

No idea, just thinking out loud. I guess OSX would reencode the tracking
branch file names just as it's messing with the normal branch file names?


Reply to this email directly or view it on GitHubhttps://github.com//pull/560#issuecomment-28210111
.

@ben
Copy link
Member Author

ben commented Nov 11, 2013

Should everything that goes through LaxUtf8Marshaler be run through String.Normalize then? I think it's reasonable to expect strings in the CLR to be normalized, since we're already converting to UTF-16. OTOH, that would break the assumption that the ref's filename should test equal to the name provided by libgit2sharp.

What OS X does to filenames I think is a separate issue, which should be dealt with at the native level (if at all).

@ethomson
Copy link
Member

I don't think you want to normalize anything in the marshaller. On any system that doesn't force a specific composition in the filesystem, you can have both of those filenames. So we if you destroyed the actual filename by normalizing to NFC we wouldn't be able to roundtrip the actual filename.

@dahlbyk
Copy link
Member

dahlbyk commented Nov 11, 2013

I think I'm with @ethomson - this test failure is an artifact of how we're checking for equality, not incorrect behavior.

@nulltoken
Copy link
Member

@ethomson @dahlbyk Ok, Thanks for your feedback!

@ben Let's go with that, then.

Could you please add an explicit comment in the test describing why we actually call string.Normalize? It'd even better if could could also add the byte[] dump as part of the comment to help the reader actually get a glimpse of what's behind this [pre]composition thingy.

then... rebasing/squashing time?

@ben
Copy link
Member Author

ben commented Nov 11, 2013

Squashed, and Works on My Machine™.

@nulltoken nulltoken merged commit 1679067 into libgit2:vNext Nov 11, 2013
@nulltoken
Copy link
Member

Squashed, and Works on My Machine™.

💖 Merged, and Works on the CI Server™, as well 💥

@dragan
Copy link

dragan commented Nov 11, 2013

Just tried these changes on my machine running Mac OS X 10.8.5 with Mono 3.2.4 and the build and tests completed without issue as well.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants