Support for BaseIntermediateOutputPath by theimowski · Pull Request #3527 · fsprojects/Paket

theimowski · 2019-03-20T12:52:19Z

This is a follow up for #2642
I didn't go the "try to parse MSBuild variable for global restore" path, but instead decided to add option to disable paket global restore.

Given following Directory.Build.props file:

<Project>

  <PropertyGroup>
    <IsWindows Condition="...">true</IsWindows>
    <IsOSX Condition="...">true</IsOSX>
  </PropertyGroup>

  <PropertyGroup>
    <DefaultItemExcludes>$(DefaultItemExcludes);$(MSBuildProjectDirectory)/obj/**/*</DefaultItemExcludes>
    <DefaultItemExcludes>$(DefaultItemExcludes);$(MSBuildProjectDirectory)/bin/**/*</DefaultItemExcludes>
    
    <!-- THIS IS NEW -->
    <PaketDisableGlobalRestore>true</PaketDisableGlobalRestore>
    <!-- THIS IS NEW -->

  </PropertyGroup>

  <PropertyGroup Condition="'$(IsWindows)' == 'true'">
    <BaseIntermediateOutputPath>$(MSBuildProjectDirectory)/obj/windows/</BaseIntermediateOutputPath>
    <BaseOutputPath>$(MSBuildProjectDirectory)/bin/windows/</BaseOutputPath>
  </PropertyGroup>

  <PropertyGroup Condition="'$(IsOSX)' == 'true'">
    <BaseIntermediateOutputPath>$(MSBuildProjectDirectory)/obj/osx/</BaseIntermediateOutputPath>
    <BaseOutputPath>$(MSBuildProjectDirectory)/bin/osx/</BaseOutputPath>
  </PropertyGroup>

</Project>

I read PaketDisableGlobalRestore property in Paket.Restore.targets :

...

    <!-- Do a global restore if required -->
    <Exec Command='$(PaketBootStrapperCommand)' Condition=" '$(PaketBootstrapperStyle)' == 'classic' AND Exists('$(PaketBootStrapperExePath)') AND !(Exists('$(PaketExePath)'))" ContinueOnError="false" />
    <Exec Command='$(PaketCommand) restore' Condition=" '$(PaketRestoreRequired)' == 'true' 
        AND '$(PaketDisableGlobalRestore)' != 'true' " ContinueOnError="false" />
    

...

which basically makes I skip global restore.
Then for each project I pass BaseIntermediateOutputPath:

    <!-- Step 3 Restore project specific stuff if required -->
    <Message Condition=" '$(PaketRestoreRequired)' == 'true' " Importance="low" Text="Detected a change ('$(PaketRestoreRequiredReason)') in the project file '$(MSBuildProjectFullPath)', calling paket restore" />
    <Exec Command='$(PaketCommand) restore --project "$(MSBuildProjectFullPath)" 
     --output-path "$(BaseIntermediateOutputPath)" --target-framework "$(TargetFrameworks)"' Condition=" '$(PaketRestoreRequired)' == 'true' AND '$(TargetFramework)' == '' " ContinueOnError="false" />
    <Exec Command='$(PaketCommand) restore --project "$(MSBuildProjectFullPath)" 
     --output-path "$(BaseIntermediateOutputPath)" --target-framework "$(TargetFramework)"' Condition=" '$(PaketRestoreRequired)' == 'true' AND '$(TargetFramework)' != '' " ContinueOnError="false" />

End game for my use case is I can build same project simultaneously on OSX and Windows and paket doesn't interfere.
I have few projects in solution, that's why skipping global restore optimisation is reasonable for me.

I'm aware my proposed solution isn't ideal, but at least it works for me and requires an explicit opt-in.
Please let me know what you think @forki @matthid @0x53A - if this is OK then I'll proceed with adding some docs and an integration test

forki · 2019-03-21T09:14:07Z

@matthid ??

matthid · 2019-03-21T10:21:36Z

This doesn't scale for more projects and introduces a perf regression, for example this most likey makes fake solution unworkable.

While this is opt in I'm not a fan, in fact this pseudo fixes the issue until one with a huge solution needs this... Personally I wouldn't merge this. But it solves someone's problem. It will be an option practically nobody has enabled and therefore we will not catch regressions easily...

Well that's my 2cents

theimowski · 2019-03-21T12:17:49Z

Do you have other ideas how to otherwise add support for BaseIntermediateOutputPath?
In #2642 you mentioned using https://github.com/enricosada/dotnet-proj-info - do you still think that's the way to go?

theimowski · 2019-03-21T13:03:39Z

Alternatively I'm thinking of finding Directory.Build.props and appending all nodes before parsing each ProjectFile

0x53A · 2019-03-21T15:44:32Z

Alternatively I'm thinking of finding Directory.Build.props and appending all nodes before parsing each ProjectFile

MsBuild is a full programming evironment in xml, that approach is going to fail for anything but the simplest files.

That's also why any "outside" parsing of project files will fail - you need msbuild. https://github.com/enricosada/dotnet-proj-info does use MsBuild and so should work correctly.

My opinion is that the perf hit would be acceptable for anything but the largest solutions with literally hundreds of projects, the restore is cached anyway. But as always with performance, it needs to be timed and profiled.

0x53A · 2019-03-21T15:50:56Z

But that was exactly why I stopped with my PR :)

matthid · 2019-03-21T16:42:49Z

My opinion is that the perf hit would be acceptable for anything but the largest solutions with literally hundreds of projects, the restore is cached anyway. But as always with performance, it needs to be timed and profiled.

Sadly this is not the problem. In fact the problem is that even while Microsoft caches its stuff Paket does not and basically last time I checked even if Microsoft cache was up to date it would still call Paket.exe and calling paket.exe is the most expensive thing in the whole process, it doesn't even matter what paket does because spawning the process and parsing command line is already too slow. So as you can see caching doesn't even matter at all.

matthid · 2019-03-21T16:53:55Z

Looking at this PR again, maybe it is acceptable:
The most important thing is to not call paket.exe a second time after the project has been restored successfully.
I think this PR still holds this property. I feel like if we add a fast-path for command line parsing for this particular call it is fine by me.
So this option will slow down the initial restore by a lot, but future restores are still fine.

theimowski · 2019-03-22T12:29:53Z

I feel like if we add a fast-path for command line parsing for this particular call it is fine by me.

Do you mean restore with --output-path argument?

If so, shall I simply remove Output_Path case from RestoreArgs and parse it directly in main ?

theimowski · 2019-03-22T12:49:49Z

Like in 25b1e8b ?

matthid · 2019-03-22T12:57:39Z

If so, shall I simply remove Output_Path case from RestoreArgs and parse it directly in main ?
Like in 25b1e8b ?

Yes, however please leave it in the argu arguments such that --help and users giving a different ordering works as expected.

theimowski · 2019-03-22T13:04:23Z

Ok - done. Added also a short note to docs
I'll add an integration tests next week (hopefully)

theimowski · 2019-03-26T13:06:14Z

@matthid This is now ready for review

forki · 2019-04-03T06:31:56Z

can you please merge with master?

theimowski · 2019-04-03T10:23:26Z

There you go

theimowski · 2019-04-03T13:34:41Z

Seems CI is telling me I broke one existing integration tests - will investigate

theimowski · 2019-04-08T11:58:11Z

The integration test fails probably due to breaking change in restore cli arguments:
I added --output-path parameter to restore in Paket.Restore.targets, but apparently integration tests are using an older build of paket.
How can I trick the netcore integration tests to use a freshly built paket?

theimowski · 2019-04-10T13:19:01Z

Think I finally got it right

forki · 2019-04-11T09:10:18Z

thanks. this is released

theimowski force-pushed the base_intermediate_output_path branch from 424a468 to 6a8ab32 Compare March 25, 2019 12:22

theimowski changed the title ~~[WIP] Support for BaseIntermediateOutputPath~~ Support for BaseIntermediateOutputPath Mar 26, 2019

theimowski force-pushed the base_intermediate_output_path branch from 821c4ce to 0c68e18 Compare April 2, 2019 14:51

theimowski force-pushed the base_intermediate_output_path branch from 0c68e18 to 8730c47 Compare April 3, 2019 10:23

theimowski added 6 commits April 8, 2019 13:35

Introduce --output-path restore arg; 'PaketDisableGlobalRestore' option

1676573

Don't use argu for restore with --output-path

5cc6e41

Add OutputPath back to RestoreArgs

a57c43f

Add section in docs

e38473a

Link PR in docs

67d7f1c

Add integration test

b83e1d5

theimowski force-pushed the base_intermediate_output_path branch from 8730c47 to b83e1d5 Compare April 8, 2019 11:36

theimowski added 2 commits April 9, 2019 14:05

Trim trailing slashes for BaseIntermediateOutputPath

38ce587

Remove double quotes for default BaseIntermediateOutputPath

ec76778

forki requested a review from matthid April 11, 2019 07:13

forki merged commit ec76778 into fsprojects:master Apr 11, 2019

theimowski deleted the base_intermediate_output_path branch April 11, 2019 09:12

Conversation

theimowski commented Mar 20, 2019

Uh oh!

forki commented Mar 21, 2019

Uh oh!

matthid commented Mar 21, 2019

Uh oh!

theimowski commented Mar 21, 2019

Uh oh!

theimowski commented Mar 21, 2019

Uh oh!

0x53A commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x53A commented Mar 21, 2019

Uh oh!

matthid commented Mar 21, 2019

Uh oh!

matthid commented Mar 21, 2019

Uh oh!

theimowski commented Mar 22, 2019

Uh oh!

theimowski commented Mar 22, 2019

Uh oh!

matthid commented Mar 22, 2019

Uh oh!

theimowski commented Mar 22, 2019

Uh oh!

theimowski commented Mar 26, 2019

Uh oh!

forki commented Apr 3, 2019

Uh oh!

theimowski commented Apr 3, 2019

Uh oh!

theimowski commented Apr 3, 2019

Uh oh!

theimowski commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theimowski commented Apr 10, 2019

Uh oh!

forki commented Apr 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0x53A commented Mar 21, 2019 •

edited

Loading

theimowski commented Apr 8, 2019 •

edited

Loading