fix app.config uptodate status

[enricosada]

fix #11

fix both Content and app.config uptodate check

app.config is like Content with "Copy if Newer", so a new build doesn't replace a modified .exe.config in output directory

[latkin]

Why don't you just add outputAssembly + ".config" to the outputs list?

[enricosada]

i handle app.config (and content items) as 1 input -> 1 output. See my comment below

[latkin]

A fix for #11 should be fairly small, and your code here basically has those required changes. However you have also included a lot of other stuff, including a redesign of how inputs and outputs are detected, some of which I don't think I agree with.

If you can trim this down to just the basic fix for #11 (with a test, please), then we will be happy to take it.

If you think the feature needs to be redesigned further (e.g. do content files have an issue?), please open an issue and provide some examples that demonstrate the bad behavior.

[enricosada]

I not added a new issue for content items because there was a repro with content items by @rojepp
I added #388 for content items ( this pr fix also #388 )

I changed more code because the problem was bigger than an app.config workaround.
The 1-1 input -> output case (app.config and Content items) was not handled

I'll add test, just tell me this pr is the way to go

The app.config is like a content item with Copy to Output Dir = Copy if newer

The uptodate code is checking output groups, but should check msbuild build items instead. That's the root cause.
This pr is (ihmo) the minimal change to make it work right, without using msbuild build items directly
Using msbuild items inputs and output metadata is the real deal, but require more work (and changes), so i skipped this way for v4.

From msbuild msdn:

Incremental builds are builds that are optimized so that targets that have output files that are
up-to-date with respect to their corresponding input files are not executed.
A target element can have both an

• Inputs attribute, which indicates what items the target expects as input
• Outputs attribute, which indicates what items it produces as output.
  MSBuild attempts to find a 1-to-1 mapping between the values of these attributes.
  If a 1-to-1 mapping exists, MSBuild compares the time stamp of every input item to the time stamp
  of its corresponding output item.
  Output files that have no 1-to-1 mapping are compared to all input files.
  An item is considered up-to-date if its output file is the same age or newer than its input file
  or files.

This mean check input output of msbuild items not source files of the project or output groups.
The output group is a vs thing ( stuff used for installer project or publish targets? ).

Not all output files are equal (there are multiple groups, see my comment on #11 ) some like Built, Symbols etc doesn't have 1 input msbuild item -> should check all source files

old code
an outputs is everything except SourceFiles group items.
We find the older output timestamp and check with newer input timestamp

This is not correct, msbuild is not going to replace the .dll or .exe, because is up to date ( an .exe/.dll input are only the Compile source files like .fs ), is going to change only the content file.

That's the real problem.

new code

The content items (and app.config as workaround because we dont check msbuild items i/o directly) are checked with the output file generated by the content item.
The old check for Built/Documentation/Symbols is the same.

If you change a content item (or not exists in output dir) this cause a build.
The next time the content output is updated, so the build is not needed.

This pr handle also the Always, PreserveNewest ( = 'Copy if newer' ) and Never ( = Do not copy) of the Copy to output directory property of Content items

[latkin]

I understand the scenario you are describing, but I don't think we should change our current behavior.

It is acceptable to have conservative detection and sometimes trigger builds that aren't strictly necessary. It is not acceptable to have aggressive detection that can sometimes fail to trigger a necessary build.

Treating all Content items that don't happen to copy into the output directory as having no role in the build falls into the latter category. Content items can represent almost anything, and it's dangerous to make assumptions about how they do/do not contribute to the build, and how they might generate corresponding outputs. The only way to have 100% fidelity in that detection is to re-implement MSBuild itself and inspect all targets, which defeats the whole purpose of the fast-up-to-date-check.

The current behavior:

• Is harmless
  • i.e. at worst it's annoying, it doesn't cause incorrect things to happen
• Is somewhat edge case
  • Only affects workflows where non-contributing Content items are updated much more frequently than actual code
• Matches C# project system behavior

C# even trips over app.config files, so it's really fine if we don't even fix that. Given that app.config are a well-known, established special case, though, I'm ok taking a scoped fix for those.

[enricosada]

ok, got it, thx @latkin for feedback 😄
I wrote a detailed reply about Content[Never] use case because i dont know how to detect it and i need help. I think, as you said, that is safer always start build when a file change (postbuild etc).
But this pr work for other Content types and app.config

I can update the pr and use the old way for Content[Never], so this pr fix

• Content[Never]
• Content[PreserveNewest]
• Content[Always]
• app.config

is this ok?

for Content[Never] we can try a fix after v4. It's a shame, .fsx are Content[None] :sad:

Do you think there is a possible workaround? Like use the dirty flag of item, check another file, dont know. /cc @KevinRansom @vasily-kirichenko @dungpa @rojepp

[latkin]

@enricosada what is the status here? Are you planning to reduce to a minimal change and add tests?

As explained earlier, there is no need to for any detection of copy semantics.

[enricosada]

@latkin didn't see this is the same behavior of c# for content items, np so.
i'll remove content items checks and leave only app.config with test. Tomorrow i think

[enricosada]

ok, summary:

• rebased for easier review
• only app.config (no content items) as request
• updated output files integration test and added a unit test
• the integration test fails before this pr ( try bc41935 ).
  • removing the .exe.config from output dir must cause a build ( fixed )
  • stale .exe.config doesn't must cause a build ( fixed )
• added check project build status with 8219016 because i got errors during test and this is safer

some notes:

pretty much the same behaviour of c#, but better ( fixed the deleted out config bug, c# got it ).

the app.config is like a content item with preserveNewest, so msbuild doesnt replace every time, only if stale ( newer is ok for msbuild )

Target "_CopyAppConfigFile" in file "C:\Program Files (x86)\MSBuild\12.0\bin\Microsoft.Common.CurrentVersion.targets" from project "D:\temp\temp projects\ConsoleApplication1\ConsoleApplication2\ConsoleApplication2.fsproj" (target "CopyFilesToOutputDirectory" depends on it):
    Skipping target "_CopyAppConfigFile" because all output files are up-to-date with respect to the input files.

The app.config need a special check ( cannot add to output items hashset ) because is always old.
Change a file.fs -> new .exe but old .exe.config. If i use the output items hashset is a problem, because the .exe.config timestamp is used for stale check -> never up-to-date until rebuild

Now the check is exe.config < app.config -> not up-to-date. So the user can modify the .exe.config and the project is up-to-date.
I can check for .exe.config = app.config, but after build the .exe.config is unchanged (msbuild doesn't replace it until rebuild) so is useless, and i prefer use the msbuild behaviour ( @latkin you call, np for me, is a = vs < 😄 )

appconfig msbuild target

  <!--
    ============================================================
                                        _CopyAppConfigFile

    Copy the application config file.
    ============================================================
    -->
  <Target Name="_CopyAppConfigFile" Condition=" '@(AppConfigWithTargetPath)' != '' " Inputs="@(AppConfigWithTargetPath)" Outputs="@(AppConfigWithTargetPath->'$(OutDir)%(TargetPath)')">
    <!--
        Copy the application's .config file, if any.
        Not using SkipUnchangedFiles="true" because the application may want to change
        the app.config and not have an incremental build replace it.
        -->
    <Copy SourceFiles="@(AppConfigWithTargetPath)" DestinationFiles="@(AppConfigWithTargetPath->'$(OutDir)%(TargetPath)')" OverwriteReadOnlyFiles="$(OverwriteReadOnlyFiles)" Retries="$(CopyRetryCount)" RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)" UseHardlinksIfPossible="$(CreateHardLinksForAdditionalFilesIfPossible)">
      <Output TaskParameter="DestinationFiles" ItemName="FileWrites" />
    </Copy>
  </Target>