Root the default output path when the current folder is unwritable

[gmipf]

Summary

DefaultOutputPath defaults to the relative "ISO", which resolves against the working directory. An installed copy has no folder next to the executable, so dumps land wherever MPF was started from — and when that folder is not writable, the dump fails.

Following the review, this keeps "ISO" as the default on every platform and only roots the path in the home directory when the current folder cannot be written to.

Change

DumpSettings.GetDefaultOutputPath(currentDirectory):

• the current folder can hold a new file → "ISO", unchanged on all platforms
• it cannot → Path.Combine(PathTool.GetHomeDirectory(), "ISO")

The probe creates a file with a random name and removes it again. Only the creation decides, so a failed cleanup does not read as unwritable. This follows GetConfigurationPath, which also tries the portable location first and moves elsewhere when writing fails. The result is cached in a static, so the probe runs once per process.

The literal now lives in one place, and OptionsLoader uses the same value as the fallback for a missing DefaultOutputPath key.

Behaviour

Nothing changes for a portable copy in a writable folder, on any platform. The fallback is reached by cases such as a portable copy unpacked into C:\Program Files\MPF, or a packaged build started with a read-only working directory.

Tests

MPF.Frontend.Test/OptionsTests.cs, four cases: writable folder, folder that cannot hold a file, missing folder, and null coercion. Inverting the writability check turns three of them red.

A real permission denial reaches the same branch. Measured on Fedora 44 against a directory at mode r-x------:

PROBE uid=gmipf dir=/tmp/c3p0urj4.ibg mode=UserExecute, UserRead -> '/home/gmipf/ISO'

That case is not in the committed tests: File.SetUnixFileMode is unsupported on Windows and CA1416 fails the build, so the shipped fixture puts a file where the directory is expected, which fails the creation on every platform and reaches the same catch.

---

Prepared with AI assistance (Claude Opus 4.8) and reviewed before submission.

[mnadareski]

I don't agree that non-Windows systems should always be treated as if they are being run from a central location. Effectively disallowing a default of the same folder for MacOS and Linux does not feel like the best path forward. I would prefer a similar situation to the existing code for the configuration, where if the current folder is unwritable, then make it elsewhere.

[gmipf]

Reworked to the shape you described. The relative "ISO" is the default on every platform again, including MacOS and Linux, and the path is only rooted when a file cannot be created in the current folder — the same approach as GetConfigurationPath, which tries the portable location first and moves elsewhere when writing fails.

The PR body is updated to describe the new behaviour. Pushed as a follow-up commit rather than a force push.

Generated with Claude Opus 4.8.

[mnadareski]

Environment.CurrentDirectory can return an empty string? Or is this just a safety check in case it's used elsewhere?

[mnadareski]

Actually, as an addendum, why does the directory need to be passed in? It's always "current directory" where it's used outside of tests.

[gmipf]

Neither, as it turns out — I measured it instead of reasoning about it, and it found a hole I had put there myself.

Environment.CurrentDirectory does not return an empty string, but it does not always return a string either. Linux allows removing the directory a process is running in, and reading the property afterwards throws:

before delete : "/tmp/mpf1013-…/nswbxtoo.ln2"
after delete  : THROWS FileNotFoundException: Unable to find the specified file.

That throw was escaping this PR. Driving the public surface in each state, one process per row because DefaultOutputPathValue caches:

current directory	master	this PR, as reviewed	with the follow-up commit
writable	ISO	ISO	ISO
unwritable	ISO	/home/<user>/ISO	/home/<user>/ISO
removed	ISO	throws FileNotFoundException	/home/<user>/ISO

master came out of a build of master, not out of reading it: the property is a constant there, so it cannot throw — which means this PR introduced the throw. The follow-up commit reads the property defensively and lets that case fall into the same branch as an unwritable directory, which is what the empty string stands for.

So the check is no longer a guard for a caller that does not exist — it is the one input the property can fail to provide. There is a test for it now, and it goes red if the check is removed (verified on both machines: 1 of the 4 fails, the other 3 stay green).

Measured on Fedora and Ubuntu, on .NET 8, 9 and 10 where the distribution ships them, with every row identical. Not measured on Windows, where a directory in use cannot be removed the same way.

Generated with Claude Opus 4.8.

[gmipf]

Correct — Environment.CurrentDirectory is the only caller outside the tests, the other four are all in OptionsTests.

What the parameter buys is that the fallback branch can be driven without touching the machine: the test points it at a path that is a file, so the creation fails on its own. Without it, a test would have to make the process's real current directory unwritable, which needs a platform-specific permission change, and set Environment.CurrentDirectory, which is process-global rather than scoped to the test. The removed-directory case from the other thread cannot be reached through the property at all: reading DefaultOutputPathValue in a writable directory and then again after moving into an unwritable one returns the first answer both times, because it caches in a static field.

So it comes down to four tests plus a parameter no production caller varies, against no parameter and the fallback branch covered only where permissions can be scripted. MPF.Frontend already carries InternalsVisibleTo("MPF.Frontend.Test"), so the seam as such is nothing new here, which is why I leaned toward keeping it — but GetConfigurationPath has no tests at all, so the precedent points the other way, and which weighs more is your call.

If you want it gone, it is a small follow-up: the method loses the argument, Environment.CurrentDirectory moves inside the try (it has to, see the other thread), and the four tests go with it.

Generated with Claude Opus 4.8.

[mnadareski]

Would attempting to create the directory itself be sufficient in testing if it can be written to? Or is there a concern that the directory may exist and but write permissions are not correct?

[gmipf]

The second one. Measured on .NET 10 (Linux, non-root), each probe scored against ground truth taken in the same state — whether a file can actually be created at <current directory>/ISO:

current dir	ISO	dump possible	Directory.CreateDirectory(ISO)	File.Create in current dir (this PR)	CreateDirectory + File.Create in ISO
writable	absent	yes	keeps relative	keeps relative	keeps relative
writable	writable	yes	keeps relative	keeps relative	keeps relative
writable	read-only	no	keeps relative	keeps relative	home
read-only	absent	no	home	home	home
read-only	writable	yes	keeps relative	home	keeps relative
read-only	read-only	no	keeps relative	home	home

Creating the directory is not sufficient: Directory.CreateDirectory returns without complaint when the directory is already there, whatever its permissions, so both rows where ISO exists read-only come back as writable and the default keeps pointing somewhere no dump can be written. Falling back to the home directory is never wrong, only cautious, so only the opposite direction is marked.

Two things about my own check while I am in there. It gets row 3 wrong for the same reason — it asks the parent, not the directory the dump goes into. And the shape you already use in GetConfigurationPath, Directory.CreateDirectory followed by File.Create, is the only one of the three that is right in all six rows.

What kept me from using it here is the side effect. Building it into GetDefaultOutputPath and constructing Options in an empty directory:

probe	directory contents after new Options()
Directory.CreateDirectory(ISO)	ISO
File.Create in current dir (this PR)	nothing

DefaultOutputPathValue is reached from a property initializer, so that ISO directory appears wherever MPF is started — including from MPF.Check, which constructs Options and never dumps. In GetConfigurationPath the file being created is the artifact the app wants; here it would be litter.

My suggestion is to leave the check as it stands: the row it gets wrong needs someone to have made ISO itself read-only inside a writable parent. If you would rather have all six covered, there are two ways — your GetConfigurationPath shape, or probing inside ISO only where it already exists, which is also six of six and leaves nothing behind for two extra lines. Say which and it goes in as a follow-up commit.

Measured on Fedora and Ubuntu; both produced the table above unchanged. Not measured on Windows, where making a directory unwritable is a different mechanism.

Generated with Claude Opus 4.8.

[mnadareski]

There is nothing wrong on the technical side of this PR. I'm just going to take a step back and see what the true utility of this is. Because the scenario that is being accounted for here could potentially be mitigated by just explicitly setting a full path default output directory in the configuration, even if the read-only "install" scenario.

This PR makes that sort of change opaque, which I'm not entirely certain is the right choice for users. It also has the potential to create the default output directory in a place you wouldn't expect just on first boot, even before the user has had a chance to update their configuration file. This could lead to an "install" scenario where the user intended on changing the default output folder.

The only cases that I can imagine that this would assist with would be users who do not pay attention to their configuration settings specifically when placed in a read-only directory. I'm not discounting this PR, I just need to think about how necessary this is to be merged.

[gmipf]

You're right, and the config layer is the correct place for this. Here's the concrete data point, since this PR came out of packaging MPF for Linux.

In that packaging, /usr/bin/mpf-cli (and the GUI and check equivalents) are thin wrapper scripts around the real binary. On every launch, before MPF starts, the wrapper writes a config if none exists, and if one exists it rewrites the output path when that path is relative — pointing it at an absolute directory under the user's home, while leaving an absolute path the user chose untouched. I checked on the installed package that this survives a manual delete: remove config.json, launch again, and it is recreated with the absolute path before MPF reads it.

So the packaged case is already covered, and covered more completely than this PR would: the wrapper sets an absolute default on every launch with no reference to the working directory, whereas this PR only relocates when the working directory is unwritable — it would still keep the relative ISO, and dump into the current folder, when that directory happens to be writable.

That leaves portable/zip users on a read-only working directory as the only ones this PR helps, which is exactly the narrow case you described. Given that, and your point about relocating opaquely on first boot, I'm going to close this — the packaging covers our own users, and I'd rather not carry a behavior change whose value is that narrow. If you'd like it kept open for the portable case, reopening is easy.

Thanks for taking the time to think it through.

Generated with Claude Opus 4.8.