-
Notifications
You must be signed in to change notification settings - Fork 68
Add Spectre.Console 0.52.0 #1447
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 Spectre.Console 0.52.0 #1447
Conversation
460c488 to
87472a0
Compare
|
After mirroring Wcwidth.Sources 3.0.0 to |
87472a0 to
3831f9a
Compare
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I triggered a run of a full unified build source-build to validate everything is working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintaining patches is non-trivial. As a follow-up I propose opening a issue/PR against Spectre.Console to see if they would be accepting of adding an msbuild property to condition this reference in order to eliminate the need for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like they would only need PolySharp for the netstandard2.0 target framework, and we don't even build that?
I will send a PR to condition it, and see what they say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch feels expensive to maintain. Any changes to these files will require regeneration of the patch. Because of the binary content, hand edits are likely off the table. What is the problem being solved here? Is this just a problem of checked in binaries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, is there a better way to do this? We could probably exclude the entire docs folder, and the build would succeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two options:
- Cloak the files from the VMR. Cloaking will prevent the files from getting mirrored to the VMR. To do this you add the path(s) to https://github.com/dotnet/dotnet/blob/main/src/source-mappings.json#L89. Here is an example.
- Add the binaries to allowed VMR binaries. Adding these files to https://github.com/dotnet/dotnet/blob/main/eng/allowed-vmr-binaries.txt will tell the system that these are known binaries allowed in the VMR. They will get stripped/removed when doing a source-build. If they are binaries required for source-build, then you would need to add them to https://github.com/dotnet/dotnet/blob/main/eng/allowed-sb-binaries.txt. As you mentioned they should be removed during source-builds.
If the files are sizable, I would lean towards cloaking. If they are minimal in number and size, allowed binaries feels appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the patch for now, and I will send a PR to dotnet/dotnet for allowed-sb-binaries.txt. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this some more, I lean towards cloaking/excluding the entire docs folder. You will want to add that exclusion in the VMR prior to merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think we should cloak them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the PR: dotnet/dotnet#3146
|
@akoeplinger I added Are there any others I missed? |
Context: dotnet/sdk#51430 Context: dotnet/source-build-reference-packages#1447 We would like to use Spectre.Console for the `dotnet` CLI, and .NET has to be able to be completely built from source. It would help us to have `PolySharp` only included when building for `netstandard2.0`, as that is the only target framework that needs the polyfills. .NET would probably only use the latest target framework. Thanks!
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Context: dotnet/source-build-reference-packages#1447 Context: dotnet/source-build-reference-packages#1287 (comment) The docs folder contains binary files, that are not needed for building from source.
|
FYI, The |
|
ok I canceled it for now. we can retrigger it once dotnet/dotnet#3146 is merged |
I force merged since the source-mappings aren't validated. Let's try again. /azp run source-build-reference-packages-unified-build |
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Did it fail because my PR here is from a fork? Or is there another error? |
Context: dotnet#1287 * Update spectre-console submodule to 0.52.0 * Add patch file to delete problematic binaries: dotnet#1287 (comment) Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
I do not understand what the issue is. @dotnet/prodconsvcs - Can you help diagnose the sync failure here? Is this AFD related? TIA |
46bb442 to
73a08a6
Compare
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@MichaelSimons I had no idea we're still using |
Looks like we need to enable PublicSign so it doesn't try to do sign. |
|
/azp run source-build-reference-packages-unified-build |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I believe this is done for the optional VMR legs that can be triggered from repo PRs. |
Context: #1287
We have a need again for Spectre.Console in .NET 11 (dotnet/sdk#51430).
Bringing back #1287, and addressing other issues.
Update spectre-console submodule to 0.52.0
Add patch file to delete problematic binaries:
#1287 (comment)