Skip to content

Commit

Permalink
[mmp] Include libmono-system-native.a when embedding mono if it exists (
Browse files Browse the repository at this point in the history 
#4980)
  • Loading branch information
@chamons
chamons authored Oct 15, 2018
1 parent 855e682 commit d29ede7
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions tools/mmp/driver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1349,13 +1349,17 @@ static int Compile ()
args.Append ("-o ").Append (StringUtils.Quote (AppPath)).Append (' ');
args.Append (cflags).Append (' ');
if (embed_mono) {
var libmono = "libmonosgen-2.0.a";
var lib = Path.Combine (libdir, libmono);
string libmono = Path.Combine (libdir, "libmonosgen-2.0.a");

if (!File.Exists (Path.Combine (lib)))
if (!File.Exists (libmono))
throw new MonoMacException (5202, true, "Mono.framework MDK is missing. Please install the MDK for your Mono.framework version from http://mono-project.com/Downloads");

args.Append (StringUtils.Quote (lib)).Append (' ');
args.Append (StringUtils.Quote (libmono)).Append (' ');

// libmono-system-native.a needs to be included if it exists in the mono in question
string libmonoNative = Path.Combine (libdir, "libmono-system-native.a");
if (File.Exists (libmonoNative))
args.Append (StringUtils.Quote (libmonoNative)).Append (' ');

var libsystem_native_path = Path.Combine (libdir, "libmono-system-native.a");
args.Append (StringUtils.Quote (libsystem_native_path)).Append (' ');

This comment has been minimized.

Sign in to view

Copy link
@alanmcgovern

alanmcgovern Nov 1, 2018

Contributor

Are we checking for the existence of the file and conditionally adding it, and then unconditionally adding the same file? This looks like an oops!

This comment has been minimized.

Sign in to view

Copy link
@chamons

chamons Nov 1, 2018

Author Contributor

I think this is right:

if (required file not there)
    throw and die

Add needed file to args

if (optional file exists)
   Add optional file

This comment has been minimized.

Sign in to view

Copy link
@chamons

chamons Nov 1, 2018

Author Contributor

Woops, did not look below the snippet:

#5073
Expand Down

4 comments on commit d29ede7

@xamarin-release-manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

❌ Build was (probably) aborted

🔥 Jenkins job (on internal Jenkins) failed in stage(s) 'Test run' 🔥 : hudson.AbortException: script returned exit code 2

✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 224 tests passed.

Failed tests

  • System.IdentityModel/watchOS - simulator/Debug: Crashed

@spouliot
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting state to success where context is continuous-integration/jenkins/branch.

No blocking issues found

@alanmcgovern
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be backported to previous release branches so we retain some form of forwards/backwards compatibility?

@baulig
Copy link
Contributor

@baulig baulig commented on d29ede7 Nov 1, 2018

Choose a reason for hiding this comment

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

@alanmcgovern Do not backport - this is not the correct way to do this, so you'd only cause conflicts. It will be done as part of the 2018-10 integration.

Please sign in to comment.