Ensure -D property passing correctly resets when -D is later removed

[lihaoyi]

The issue here is that we were not properly resetting the system property when -D was removed, resulting in it incorrectly hanging around with the last set value.

This PR stores the initial value of every system property, along with the keys for the system properties set with -D for each run. Now when a -D key-value pair is removed, we can compare the key against the initialSystemProperties to decide whether we need to either remove the key value or to restore it to its original value

Tested manually using a simple build.sc:

val foo = interp.watchValue(sys.props.get("foo"))

val osname = interp.watchValue(sys.props.get("os.name"))

def bar() = T.command{ println(s"$foo $osname") }

On main, removing -D flags leaves the properties hanging around with the last set value:

lihaoyi mill$ ./mill -i dev.run ../test -D foo=a -D os.name=lols bar
...
Some(a) Some(lols)

lihaoyi mill$ ./mill -i dev.run ../test  bar
...
Some(a) Some(lols)

With this PR, removing the `-D flags has the properties either cleared or reset to their initial value:

lihaoyi mill$ ./mill -i dev.run ../test -D foo=a -D os.name=lols bar
...
Some(a) Some(lols)

lihaoyi mill$ ./mill -i dev.run ../test  bar
...
None Some(Mac OS X)

Review by @lefou

[lihaoyi]

Not sure what the mima tests are complaining about, but the failure in the CaffeineTests looks like a flake

[lefou]

Not sure what the mima tests are complaining about, but the failure in the CaffeineTests looks like a flake

Changing a case class is always binary incompatible. This is OK I think in your case. We currently check all modules, to be aware of any change.

[lefou]

Evaluator is part of the API which is used in build.sc (no binary issue, as this is always recompiled) but also by plugins. Maybe, we should provide a compatibility apply/copy method. And deprecate it immediately.

[lihaoyi]

@lefou we're only touching Evaluator.State though. Do any plugins really instantiate this? I mean in theory it's not private, but we've never been disciplined about privatizing things in the past. This definitely seems like something I expect Mill to use and nobody else

[lefou]

Evaluator is part of the documentation (https://com-lihaoyi.github.io/mill/mill/Extending_Mill.html#_evaluator_commands_experimental) and also used outside mill main/core, e.g. contrib plugins make use of it, but probably also thirdparty plugins (e.g. scala steward). As we copy the evaluator, it's fair th assume, others do it, too.

I belief providing a nice binary compatibility story is one of the (many) fundatental things to create a nice user experience. It's hard to get right and easily to screw up. Deprecation is a nice tool to show API removal/changes before it's too late.

[lefou]

@lihaoyi Sorry, I didn't looked carefully. You're right, Evaluator.State can be considered as internal.

[lefou]

Looks good to me!