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

Fixes 3352 - copy local has no effect when opening project in vs2017 #3356

Merged
merged 5 commits into from
Sep 20, 2018
Merged

Fixes 3352 - copy local has no effect when opening project in vs2017 #3356

merged 5 commits into from
Sep 20, 2018

Conversation

atlemann
Copy link
Contributor

@atlemann atlemann commented Sep 11, 2018 •
edited
Loading

Fixes: #3352

Setting <ExcludeAssets>runtime</ExcludeAssets> in the .paket.props file for packages with copy_local: false to avoid VS2017 changing the props.assets.json when loading the project.

Tweaked the result of combining copy_local flags from lock and references file:

  • Setting only copy_local: true returns Some true which results in PrivateAssets=All
  • Combinations of copy_local: true and copy_local: false results in either Some false or None which results in either ExcludeAssets=runtime or no flags at all.

Moved the copy_local setting to element 5 in the .paket.resolved file in the obj folder and updated paket.Restore.targets to read the 5th element instead.

@atlemann
Add ExcludeAssets=runtime in .paket.props when copy_local: false,
238f938 
to avoid VS2017 to change the project.assets.json when opening a project
Excract function for combining copy_local from lock and references files
@atlemann
Inline the CombineCopyLocal check
5e65076
@atlemann
Copy link
Contributor Author

So, what's up with the Travis build? Does it break from time to time or did my changes break something?

@atlemann
Copy link
Contributor Author

atlemann commented Sep 12, 2018 •
edited
Loading

Why I'm asking about PrivateAssets is that I have a scenario where I have copy_local: false for some packages in the paket.dependencies file and copy_local: true for some of them in some paket.references files (in this case for unit-test projects). This results in PrivateAssets=All being set for those packages. Why is the copy_local flag used to set PrivateAssets=All when it is ExcludeAssets=runtime that enables the expected behavior?

@atlemann
Copy link
Contributor Author

Other than the PrivateAssets thing, the changes committed fixes my issue. Should I remove the WIP and open another issue with the PrivateAssets thing instead?

@atlemann
Copy link
Contributor Author

I guess this is why: #2951

@atlemann
Copy link
Contributor Author

Maybe I could change the code to only consider copy_local: true in the paket.depencencies file to return true to the .paket.props file. If copy_local: true is set in the paket.references file, I could just return false which wouldn't set either PrivateAssets or ExcludeAssets, which for me is the expected behavior. It would be kind of confusing though.

@atlemann atlemann changed the title [WIP] Fixes 3352 - copy local has no effect when opening project in vs2017 Fixes 3352 - copy local has no effect when opening project in vs2017 Sep 13, 2018
@atlemann
Set PrivateAssets=All if only copy_local:true is set for a package in…
373c969 
… either lock or references file

Separated the PrivateAssets and CopyLocal settings in .paket.resolved
Added CopyLocal tag in paket.Restore.targets which reads the new column
@atlemann
Copy link
Contributor Author

I'm done with this one as it fixes the problem I had with opening projects in VS2017 with copy_local: false.

@atlemann
Copy link
Contributor Author

Unless I have to change the PaketPropsVersion in RestoreProcess.fs since I'm now editing the .paket.props file

@atlemann
Copy link
Contributor Author

@forki @matthid Sorry for bothering you, but these changes fixes the issues I got when converting a C# solution with 180 projects (I know, I want to hang myself) to new SDK format. We're using the copy_local option quite extensively and would need this fix or something similar since VS2017 is changing the runtime entries in the project.assets.json when it loads the projects. This results in the .dlls with copy_local: false to be copied to the bin folders anyway.

Is there another way to fix this I should try instead?
Do you have some advice on how to add a test for this?

Running build.sh is all green locally, so I'm not sure why the builds are failing here.

@matthid
Copy link
Member

matthid commented Sep 17, 2018

@atlemann Thanks for the PR.

Regarding the Code: I think there should be a built-in to get the "package settings" after considering the paket.references file. So I'm a bit hesitant about doing that manually.

Regarding the Test-Suite: Yes it is probably broken again. I assume at least appveyor should be green again after adding the netcoreapp2.2 changes to the integration test suite (/cc @forki )

@atlemann
Copy link
Contributor Author

Um, not sure I follow what you mean by built-in?

@atlemann
Copy link
Contributor Author

@matthid Is there anything I can change to make it mergable? I basically have to postpone switching to new SDK format due to this.

@forki
Copy link
Member

forki commented Sep 19, 2018

please merge with master

@atlemann
Copy link
Contributor Author

At least the Appveyor build succeeded again now

@forki forki merged commit 9a68591 into fsprojects:master Sep 20, 2018
@forki
Copy link
Member

forki commented Sep 20, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

copy_local: false has no effect when opening .NET SDK projects with VS2017
3 participants
@atlemann @matthid @forki