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

AppSettings 'file' attribute supports a relative path to the target of a... #1489

Closed
wants to merge 1 commit into from
Closed

AppSettings 'file' attribute supports a relative path to the target of a... #1489

wants to merge 1 commit into from

Conversation

ninjarobot
Copy link
Contributor

... symlink to the .config on *nix systems.

Added unit test to check that the 'file' attribute in App.config resolves to a .config file at a relative path to the symlink target .config.

This change is released under the MIT license.

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@ninjarobot
Copy link
Contributor Author

@akoeplinger - I created a new PR for this to cut all the cruft from the prior one. Please review and comment.

@akoeplinger
Copy link
Member

@ninjarobot Did you try this on a clean checkout? I don't see any changes to the Makefile for Mono.Posix like we talked about in the other PR.

@ninjarobot
Copy link
Contributor Author

@akoeplinger I really haven't been able to determine how to apply what you're suggesting to build multiple times. My makefile skills have been exceeded. :)

No matter what I do, I end up with something like: "*** No rule to make target `../../class/lib/build/bare/Mono.Posix.dll'"

@akoeplinger
Copy link
Member

@ninjarobot I managed to make it build with this patch:

diff --git a/mcs/class/Mono.Posix/Makefile b/mcs/class/Mono.Posix/Makefile
index 03af467..911259e 100644
--- a/mcs/class/Mono.Posix/Makefile
+++ b/mcs/class/Mono.Posix/Makefile
@@ -3,6 +3,7 @@ SUBDIRS =
 include ../../build/rules.make

 LIBRARY = Mono.Posix.dll
+LOCAL_MCS_FLAGS = -lib:$(bare_libdir)
 # Don't warn about [Obsolete] members, as there are now *lots* of [Obsolete]
 # members, generating volumes of output.
 LIB_MCS_FLAGS = /unsafe /r:$(corlib) /r:System.dll /nowarn:0618,612
diff --git a/mcs/class/System.Configuration/Makefile b/mcs/class/System.Configuration/Makefile
index e5838dc..abb4387 100644
--- a/mcs/class/System.Configuration/Makefile
+++ b/mcs/class/System.Configuration/Makefile
@@ -15,7 +15,8 @@ include ../../build/library.make
 configuration_library_deps = \
        $(secxml_libdir)/System.dll     \
        $(the_libdir_base)System.Security.dll   \
+       $(the_libdir_base)Mono.Posix.dll  \
        $(bare_libdir)/System.Xml.dll

 $(build_lib): $(configuration_library_deps)

@@ -28,5 +29,8 @@ $(secxml_libdir)/System.dll:
 $(the_libdir_base)System.Security.dll:
        (cd ../System.Security; $(MAKE) $@)

+$(the_libdir_base)Mono.Posix.dll:
+       (cd ../Mono.Posix; $(MAKE) $@)
+
 $(bare_libdir)/System.Xml.dll:
        (cd ../System.XML; $(MAKE) $@)

@ninjarobot
Copy link
Contributor Author

@akoeplinger awesome, yes that takes care of it, thank you very much! is seems this is what was missing: LOCAL_MCS_FLAGS = -lib:$(bare_libdir)

@akoeplinger
Copy link
Member

whitelist

@ninjarobot
Copy link
Contributor Author

Fixed the failing tests. One was a problem with the test logic, but the other was an error in the parameter to UnixPath.GetRealPath. Again, many thanks to @akoeplinger for chatting through issues.

Console.WriteLine(pwd);
var p = Process.Start (new ProcessStartInfo {
FileName = "mono",
Arguments = "TestLoadsFromFileAttribute.exe" // Path.Combine (pwd, "../TestLoadsFromFileAttribute.exe"),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some leftover comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akoeplinger thanks for catching that. Removed it.

…f a symlink to the .config on *nix systems.

Added unit test to check that the 'file' attribute in App.config resolves to a .config file at a relative path to the symlink target .config.

This change is released under the MIT license.
@ninjarobot
Copy link
Contributor Author

@akoeplinger I'm not sure what to do about this merge build failure. It appears completely unrelated. Is there a known problem with CI?

@akoeplinger
Copy link
Member

@ninjarobot yes it is unrelated, the CI is a bit unstable right now.

@kumpera
Copy link
Contributor

kumpera commented Jan 21, 2015

Couple of issues.

We can't have System depending on Mono.Unix, not all targets support it.

Second, this looks like a breaking change. Existing users of symlinks would get a different behavior.

Why is the new behavior better?

@ninjarobot
Copy link
Contributor Author

@kumpera thank you for the feedback. It's System.Configuration, not System. Is there something I need to adjust in the makefile so that it's only linked on certain targets? Can you possibly point me to an example in another makefile? I'm already accounting for this in the code itself, so it's only going to use Mono.Unix on PlatformID.Unix or PlatformID.MacOSX.

The change isn't breaking. It saying to follow get the real path to find the configuration file (my application.exe.config) including following any symlinks which are ignored today. The AppSettings 'file' attribute is documented to be a relative path to the .config file, which works fine under mono unless your path to a .config file is a symlink. If you don't have a symlink to the .config file, it continues to work as before, but if you happen to be using symlinks, then only with this fix will it still find the AppSettings 'file' relative to the real file.

The reason this is better is because I like to be a good neighbor on Unix systems. My application is stored under /usr/lib/my/app.exe and my config files are under /etc/my/app.exe.config, where a sysadmin would expect to find things. The documentation specifies the .config file should be next to the .exe, so I put a symlink /usr/lib/my/app.exe.config that links to the /etc/my/app.exe.config file. All is well until I happen to use <appSettings file="more.config" />, and instead of looking in /etc/my/more.config, it looks in /usr/lib/my/more.config (next to the symlink instead of the real file). So I want to fix that.

Existing users of symlinks have to do what I do today, which is hardcode the full path to the more.config file, as in <appSettings file="/etc/my/more.config" />. That's OK, but it's not what the documentation says the behavior of the 'file' attribute should be, and it's inconvenient if you run multiple copies of an application with different configurations or anything like that because you have to edit this full path. Since anyone who does this now would have the full path in their 'file' attribute, their application isn't going to break.

I hope that helps explain the benefit. Please let me know if I can provide more information or what my options are for targets that don't support Mono.Unix.

@alexrp
Copy link
Contributor

alexrp commented Mar 16, 2015

@kumpera ping? Is there any reason the dependency on Mono.Posix.dll is problematic when it's guarded like this?

@ninjarobot
Copy link
Contributor Author

It looks like all of my test cases for retrieving symlink'd configs work with the current code now. As @kumpera suggested, it seems to follow symlinks properly automatically. I'm probably going to just close this PR unless @alexrp or someone else is having an issue.

@akoeplinger
Copy link
Member

@ninjarobot so this suddenly started working? We should then split off your tests in a new PR to ensure it doesn't break in the future.

@ninjarobot
Copy link
Contributor Author

@akoeplinger not exactly. It started working when this was fixed, which happened between my initial PR and when I was asked to resubmit it.

@akoeplinger
Copy link
Member

@ninjarobot anyway, it'd be great to split off the tests in a separate PR, then we can close this here.

@ninjarobot
Copy link
Contributor Author

@akoeplinger I will split them off and close this one. Thanks for your help.

@lewurm
Copy link
Contributor

lewurm commented Mar 28, 2016

@ninjarobot is this PR okay to close?

@ninjarobot
Copy link
Contributor Author

@lewurm yes, not needed anymore, will close.

@ninjarobot ninjarobot closed this Apr 1, 2016
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.

None yet

7 participants