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.

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

Update YamlDotNet assemblies to 13.0, add support for pwsh 7.x #97

Merged
merged 6 commits into from
Feb 12, 2023
Merged

Conversation

Eggdale
Copy link
Contributor

@Eggdale Eggdale commented Feb 9, 2023

Howdy,

I've updated the internal YamlDotNet assemblies to 13.0 to fix #84, which allows for fixing #32 by adding the WithIndentedSequences option to ConvertTo-Yaml. The YamlDotNet.Serialization.SerializationOptions enum was removed in this YamlDotNet issue, so I added it back locally.

Additionally, I've added a fix for pwsh 7.x which would prevent the tests from running due to pwsh not automatically loading standard references when those are loaded manually. see here for details.

Thanks!

@gabriel-samfira
Copy link
Member

gabriel-samfira commented Feb 9, 2023

Hi @Eggdale !

This is awesome! Thank you!

I have a couple of questions. Where did you download the DLLs from? Which build?

The current release (https://github.com/aaubry/YamlDotNet/releases/tag/v13.0.0), lists aaubry/YamlDotNet#771 as the PR which merged last into this release. This corresponds to this build: https://ci.appveyor.com/project/aaubry/yamldotnet/builds/46071641/artifacts.

The sha256sum from the above build with the ones in your branch, seem to differ.

Thanks again for taking the time to make these changes!

@Eggdale
Copy link
Contributor Author

Eggdale commented Feb 10, 2023

Interesting, I pulled them out of the nuget package from nuget package repository. I assumed those would be the same as the release package but they're not. Even the nuget package on the appveyor site has different binaries.

I've added a commit to use the release versions instead of those nuget versions.

Thanks!

@gabriel-samfira
Copy link
Member

Hi @Eggdale !

The ones in your commit have the following sha256 sums:

gabriel@arrakis:/tmp/powershell-yaml$ git remote -v
origin	https://github.com/Eggdale/powershell-yaml.git (fetch)
origin	https://github.com/Eggdale/powershell-yaml.git (push)
gabriel@arrakis:/tmp/powershell-yaml$ git log -n 1
commit 3937fea6aa762336ea9ce23038a4b82e0f0aa92f (HEAD -> master, origin/master, origin/HEAD)
Author: Eggdale <[email protected]>
Date:   Thu Feb 9 18:56:06 2023 -0600

    Updated YamlDotNet to release version
gabriel@arrakis:/tmp/powershell-yaml$ sha256sum lib/*/YamlDotNet.dll
94f4ccbb6ea28cfb10fcfa3aa13109e4c7b3efd6aa42f55dae6e0f8fb55914b7  lib/net35/YamlDotNet.dll
9af8026704a0f7f15491fccea4e1e3aee84ec7d22cdf361ba3395b1109968960  lib/net45/YamlDotNet.dll
9623827afa09c0f504282088bccf929ec92b5ff425515e2e5740599829493850  lib/netstandard2.1/YamlDotNet.dll

The ones from https://ci.appveyor.com/project/aaubry/yamldotnet/builds/46071641/artifacts have the following sha256 sums:

gabriel@arrakis:/tmp/tmp.rvk8YB3oeC$ wget -q -O YamlDotNet.13.0.0.zip https://ci.appveyor.com/api/buildjobs/h62oyyp2v4icscna/artifacts/YamlDotNet%2Fbin%2FYamlDotNet.13.0.0.nupkg
gabriel@arrakis:/tmp/tmp.rvk8YB3oeC$ unzip -q YamlDotNet.13.0.0.zip 
gabriel@arrakis:/tmp/tmp.rvk8YB3oeC$ sha256sum lib/{netstandard2.1,net35,net45}/YamlDotNet.dll
c24933cb18f83e97fcae76c4baa6909964bea4b88906bff3cd4bb5e923da469a  lib/netstandard2.1/YamlDotNet.dll
183f985548d83691e5dba0670a38d13f07f5f838ed90a71b3f7516ebebcef05d  lib/net35/YamlDotNet.dll
321b606038c1b447977a996f8c77ea9c9b7cb20fd8eaf9232e9d9ca9dc24d2fa  lib/net45/YamlDotNet.dll

Sorry to nitpick, I just want to make sure we get the right version in.

@Eggdale
Copy link
Contributor Author

Eggdale commented Feb 10, 2023

No worries, thanks for keeping me honest!
Two things happened here: I used a more recent build for this update than the one that you list in your comment, https://ci.appveyor.com/project/aaubry/yamldotnet/builds/46172294/artifacts, as well as pulling from the bin\Release folder instead of extracting the NuGet package. I originally submitted with the extracted NuGet package as it's what I already had on my machine, but the newest commit is using the Release binaries. I see those sums matching the ones in my commit:

eggdale@DESKTOP-UPL75UE:~$ wget -q -O Release-Net35.zip https://ci.appveyor.com/api/buildjobs/2adpm5ej1g96e1ss/artifacts/YamlDotNet%2Fbin%2FRelease%2FRelease-Net35.zip
eggdale@DESKTOP-UPL75UE:~$ unzip -q Release-Net35.zip
eggdale@DESKTOP-UPL75UE:~$ sha256sum YamlDotNet.dll
94f4ccbb6ea28cfb10fcfa3aa13109e4c7b3efd6aa42f55dae6e0f8fb55914b7  YamlDotNet.dll

eggdale@DESKTOP-UPL75UE:~$ wget -q -O Release-Net45.zip https://ci.appveyor.com/api/buildjobs/2adpm5ej1g96e1ss/artifacts
/YamlDotNet%2Fbin%2FRelease%2FRelease-Net45.zip
eggdale@DESKTOP-UPL75UE:~$ unzip -q Release-Net45.zip
eggdale@DESKTOP-UPL75UE:~$ sha256sum YamlDotNet.dll
9af8026704a0f7f15491fccea4e1e3aee84ec7d22cdf361ba3395b1109968960  YamlDotNet.dll

eggdale@DESKTOP-UPL75UE:~$ wget -q -O Release-NetStandard-2.1.zip https://ci.appveyor.com/api/buildjobs/2adpm5ej1g96e1ss/artifacts/YamlDotNet%2Fbin%2FRelease%2FRelease-NetStandard-2.1.zip
eggdale@DESKTOP-UPL75UE:~$ unzip -q Release-NetStandard-2.1.zip
eggdale@DESKTOP-UPL75UE:~$ sha256sum YamlDotNet.dll
9623827afa09c0f504282088bccf929ec92b5ff425515e2e5740599829493850  YamlDotNet.dll

Should I use a different build, or is this one sufficient?

Sorry for the confusion, still very green when it comes to open source contributions.

@gabriel-samfira
Copy link
Member

gabriel-samfira commented Feb 11, 2023

Sorry for the confusion, still very green when it comes to open source contributions.

No worries at all! We've all started somewhere 😄.

In general, when dealing with binary blobs, it's important to state the exact location where they were downloaded from and versions. This helps trust and allows anyone that wants to use this project to verify the binaries for themselves before using it on production systems. In general, anything that is not code that can be audited, must be verifiable.

When it comes to projects that have wide usage (approximately 1.2 mil downloads/month in this case), we must be extra careful and transparent 🙂 .

That being said, let's use the binaries from the latest stable release, which seems to be this one: https://ci.appveyor.com/project/aaubry/yamldotnet/builds/46071641/artifacts (version: 13.0.0.944).

@Eggdale
Copy link
Contributor Author

Eggdale commented Feb 12, 2023

Thanks for the guidance, I've updated the binaries accordingly.

Thanks!

@gabriel-samfira
Copy link
Member

One last thing, which I should have noticed earlier. While the dlls are built from an open source and free project, we are still obliged to acknowledge where we got them from and distribute them with their license included in each folder (lib/netstandard2.1, lib/net35 and lib/net45) . You can copy the license from:

https://github.com/aaubry/YamlDotNet/blob/master/LICENSE.txt
https://github.com/aaubry/YamlDotNet/blob/master/LICENSE-libyaml

Sorry I didn't catch it earlier, and thanks for your patience 😄.

@Eggdale
Copy link
Contributor Author

Eggdale commented Feb 12, 2023

Gotcha, makes sense. I didn't see them with the releases in the new version so I wasn't sure whether or not to include them manually.

Thanks!

@gabriel-samfira gabriel-samfira merged commit 57df466 into cloudbase:master Feb 12, 2023
@gabriel-samfira
Copy link
Member

Gotcha, makes sense. I didn't see them with the releases in the new version so I wasn't sure whether or not to include them manually.

Thanks!

They are not usually included in the binary builds. If we're simply consuming them for our needs, we don't really need to keep the license text with the binaries, but we're redistributing them in our own project, so we have to do that.

Thanks for taking the time to contribute these changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update YamlDotNet?
2 participants