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.

Sign up for GitHub

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

Jump to bottom

Support for BaseIntermediateOutputPath #3527

Merged
merged 8 commits into from
Apr 11, 2019
Merged

Support for BaseIntermediateOutputPath #3527

merged 8 commits into from
Apr 11, 2019

Conversation

theimowski
Copy link
Member

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
Copy link
Member

forki commented Mar 21, 2019

@matthid ??

@matthid
Copy link
Member

matthid commented Mar 21, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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

@0x53A
Copy link
Contributor

0x53A commented Mar 21, 2019 •
edited
Loading

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
Copy link
Contributor

0x53A commented Mar 21, 2019

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

@matthid
Copy link
Member

matthid commented Mar 21, 2019

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
Copy link
Member

matthid commented Mar 21, 2019

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
Copy link
Member Author

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
Copy link
Member Author

Like in 25b1e8b ?

@matthid
Copy link
Member

matthid commented Mar 22, 2019

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
Copy link
Member Author

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

@theimowski theimowski force-pushed the base_intermediate_output_path branch from 424a468 to 6a8ab32 Compare March 25, 2019 12:22
@theimowski theimowski changed the title [WIP] Support for BaseIntermediateOutputPath Support for BaseIntermediateOutputPath Mar 26, 2019
@theimowski
Copy link
Member Author

@matthid This is now ready for review

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

forki commented Apr 3, 2019

can you please merge with master?

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

There you go

@theimowski
Copy link
Member Author

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

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

theimowski commented Apr 8, 2019 •
edited
Loading

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
Copy link
Member Author

Think I finally got it right

@forki forki requested a review from matthid April 11, 2019 07:13
@forki forki merged commit ec76778 into fsprojects:master Apr 11, 2019
@forki
Copy link
Member

forki commented Apr 11, 2019

thanks. this is released

@theimowski theimowski deleted the base_intermediate_output_path branch April 11, 2019 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid Awaiting requested review from matthid

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@theimowski @forki @matthid @0x53A