Skip to content

Conversation

@chamons
Copy link
Contributor

@chamons chamons commented Oct 10, 2016

Also includes some test infrastructure improvements that can split out if necessary.

…M apps

- https://bugzilla.xamarin.com/show_bug.cgi?id=44707
- Also teach mmp about MonoBundle/mono/4.5/machine.config file
- Mono should be fixed to not need machine.config, but may come up in future
- Also allow customers to copy in custom machine.config easily
- Also clean up some test output formatting issues found while writing new tests
@chamons
Copy link
Contributor Author

chamons commented Oct 10, 2016

@kumpera - This is the XM side of 44707. We're not generating an "empty" machine.config since you fixed mono, but the option is there --custom-machine-config="" in case the bug resurfaces.

@timrisi Please look over the test changes.

@rolfbjarne The launcher changes are trivial, but can you verify they are in the right place?

@chamons
Copy link
Contributor Author

chamons commented Oct 10, 2016

The XM_TEST_NAME hack is something i've wanted to do for awhile, so I can stop locally hacking that file every time one test fails.

setenv ("MONO_DEBUG", "no-gdb-backtrace", 0);
}

setenv ("MONO_CFG_DIR", [monobundle_dir UTF8String], 1);
Copy link
Member

Choose a reason for hiding this comment

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

We already call mono_set_dirs [1], whose second parameter sets the config directory. Maybe we should check why that doesn't work instead?

[1] https://github.com/xamarin/xamarin-macios/blob/master/runtime/launcher.m#L495-L499

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look into this. That is a good point.

Copy link
Contributor Author

@chamons chamons Oct 10, 2016

Choose a reason for hiding this comment

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

@rolfbjarne @kumpera

We first run mono_set_dirs here:

frame #0: RemotingTest`mono_set_dirs()
frame #1: RemotingTest`app_initialize()
frame #2: RemotingTest`::xamarin_main()
frame #3: RemotingTest`main()

and appear to correctly set things. But then in mono_main we get stomped

frame #0: RemotingTest`fallback [inlined] mono_set_dirs(assembly_dir="/Users/donblas/Programming/macios/master/xamarin-macios/builds/install/mac64/lib", config_dir="/Users/donblas/Programming/macios/master/xamarin-macios/builds/install/mac64/etc") at assembly.c:588 [opt]
frame #1: RemotingTest`fallback + 23
frame #2: RemotingTest`mono_set_rootdir
frame #3: RemotingTest`mono_main() 
frame #4: RemotingTest`::xamarin_main()
frame #5: RemotingTest`main()

And then we're hosed.

MONO_CFG_DIR is letting us bypass this.

Is this a bug/limitation in the embedding API? It seems wrong.

Copy link
Member

@rolfbjarne rolfbjarne Oct 11, 2016

Choose a reason for hiding this comment

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

@chamons OK, I guess we can fix this ourselves until mono fixes its embedding API then.

But maybe pass 0 for the overwrite parameter to setenv then, so that the value won't overwrite any existing values set if executed from the command line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed a bug w\ mono here - https://bugzilla.xamarin.com/show_bug.cgi?id=45279

I'll change that setenv param now.

{ "allow-unsafe-gac-resolution", "Allow MSBuild to resolve from the System GAC", v => {} , true }, // Used in Xamarin.Mac.XM45.targets and must be ignored here. Hidden since it is a total hack. If you can use it, you don't need support
{ "disable-lldb-attach=", "Disable automatic lldb attach on crash", v => disable_lldb_attach = ParseBool (v, "disable_lldb_attach")},
{ "disable-lldb-attach=", "Disable automatic lldb attach on crash", v => disable_lldb_attach = ParseBool (v, "disable-lldb-attach")},
{ "custom-machine-config=", "Custom machine.config file to copy into MonoBundle/mono/4.5/machine.config. Pass \"\" to copy in a valid \"empty\" config file.", v => custom_machine_config_path = v },
Copy link
Member

Choose a reason for hiding this comment

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

Isn't custom- a bit redundant? If you're passing a path here, it's kind of obvious it's a custom file... so maybe name it just machine-config= instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest to just --machine-config

@monojenkins
Copy link
Collaborator

Build failure

1 similar comment
@monojenkins
Copy link
Collaborator

Build failure

@timrisi
Copy link
Contributor

timrisi commented Oct 10, 2016

Test changes lgtm

@monojenkins
Copy link
Collaborator

Build failure

1 similar comment
@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@chamons
Copy link
Contributor Author

chamons commented Oct 12, 2016

Hmm. Somehow i broke msbuild-tests. Will look into it.

- FSharp tests will fail to build inside msbuild-test with:
   /Library/Frameworks/Mono.framework/Versions/4.4.0/lib/mono/4.5/Microsoft.FSharp.Targets: error : Error executing task Fsc: ToolPath is unknown; specify the path to fsc.exe as the ToolPath property.
- So we unset MONO_CFG_DIR when we setup the other environomental variables, which was setup by the test launcher in launcher.m
@monojenkins
Copy link
Collaborator

Build success

- Remove hack in tests since we've moved into all apps
- Was breaking xbuild building fsharp inside any XM app
@monojenkins
Copy link
Collaborator

Build success

@chamons chamons merged commit 17bb354 into master Oct 13, 2016
@chamons chamons deleted the xm_44707 branch October 13, 2016 15:42
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