-
Notifications
You must be signed in to change notification settings - Fork 702
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.
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
Add option to serialize global properties when running static graph-based restore #5413
Add option to serialize global properties when running static graph-based restore #5413
Conversation
src/NuGet.Core/NuGet.Build.Tasks/GetReferenceNearestTargetFrameworkTask.cs
Show resolved
Hide resolved
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
507a9f7
to
fe39645
Compare
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
df24c15
to
164d30b
Compare
This implementation no longer attempts to enable the setting and instead logs a useful error message that users can use to find the solution of enabling the option. |
I am sure you may have already explored this option, but I am curious to know the possibility of using response files as suggested by Chet and Jared in NuGet/Home#11968 (comment). |
The problem with response files is NuGet would need to write it to disk, then it would be read by the new process, then it would need to be deleted. Writing to the current directory seems bad since the data in this response is only ever meant to be read by our process. Writing to TEMP has been problematic in the past on other platforms so I avoided that. If this was a process meant to be launched by users, it would absolutely make sense to use a response file that the user could author. But in this case its just one process launching another process and sending data to it. STDIN made sense to use so we don't have to deal with a location to write to disk and it should technically be more performant. |
1623a92
to
15c9aff
Compare
Bug
Fixes: NuGet/Home#11968
Regression? Last working version:
Description
Adds an option to allow a user to specify that global properties should be serialized over STDIN instead of passed as command-line arguments. If the process fails to start, an error is logged so users can find information about how to enable the property. In future versions, we can explore enabling this option by default.
RestoreSerializeGlobalProperties
that the user can set totrue
if they want to opt-in to the functionalityConsole.InputEncoding
to avoid the original problem where a preamble is written to the streamNuGet.Build.Tasks.Console
writes errors to STDERR as strings andRestoreTaskEx
logs them.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation