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

Stop synthesizing owners #3241

Merged
merged 2 commits into from May 18, 2020
Merged

Stop synthesizing owners #3241

merged 2 commits into from May 18, 2020

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 16, 2020

Bug

Fixes: NuGet/Home#5134
Regression: No

Fix

Details: <owners> is no longer forced to appear when not specified.

Testing/Validation

Tests Added: Yes
Validation: Integration tests failed before fix, passed after fix

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 18, 2020

Also: if you'd like me to take a different approach or fit other constraints, I'd be glad to try.

@zivkan zivkan added the Community PRs created by someone not in the NuGet team label Mar 5, 2020
@nkolev92 nkolev92 self-assigned this Mar 6, 2020
@nkolev92
Copy link
Member

nkolev92 commented Mar 6, 2020

@jnm2

Thgere are bunch of failing tests 68 in total, I'll paste the class names as our CI is private.
Look at tests like:
PackCommand_ExcludesFilesOutsideRoot
PackCommand_PackageFromNuspecWithEmptyFilesTag
PackCommand_WhenPackingSymbolsPackage_ExcludesExplicitAssemblyReferences
PackCommand_WarningDependencyVersionNotSpecified
PackCommand_IncludeExcludePackageFromNuspec
PackCommand_PackIcon_HappyPath_Succeeds
PackCommand_ContentV2PackageFromNuspec
PackCommand_SemVer200_Succeeds
PackCommand_MultipleFrameworks_GeneratesPackageOnBuild

PackCommand_NewProject_AddsTitleToNuspec
Icon_HappyPath_Suceed
PackageBuilderThrowsWhenLicenseUrlIsMalformed
ReadingPackageManifestRecognizeMultipleDependenciesWithTargetFramework

Can you please take a look?
A bunch of them are null refs.

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 23, 2020

@nkolev92 Right now it appears that nuspecs without an <owners> element or with an empty <owners> element do not roundtrip. Instead, the element shows up equal to the <authors> element.

Is it acceptable to make it so that nuspecs without an <owners> element do roundtrip for the first time, and that nuspecs with an empty <owners> element still don't roundtrip but they result in no <owners> element instead of resulting in an <owners> element that copies the <authors> element?

@jnm2 jnm2 force-pushed the stop_synthesizing_owners branch 2 times, most recently from 0b711e8 to ba82270 Compare March 23, 2020 03:45
@jnm2
Copy link
Contributor Author

jnm2 commented Mar 23, 2020

I believe I've fixed all the tests you mentioned and found some more related ones to fix. Could you rerun CI and let me know?

@zivkan
Copy link
Member

zivkan commented Mar 27, 2020

The windows functional test and mac functional test failures were the same, just on different platforms:

NuGet.CommandLine.Test.NuGetSpecCommandTests.SpecCommand_NoProjectFile
NuGet.CommandLine.Test.NuGetSpecCommandTests.SpecCommand_NoProjectFile_WithArg
NuGet.CommandLine.Test.NuGetSpecCommandTests.SpecCommand_WithProjectFile

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 27, 2020

Thanks, pushed fixes for those three.

@nkolev92
Copy link
Member

Is it acceptable to make it so that nuspecs without an element do roundtrip for the first time, and that nuspecs with an empty element still don't roundtrip but they result in no element instead of resulting in an element that copies the element?

I think it's ok. There's unfortunately no neat way to handle all this in our libraries, so this is the best we have.

@nkolev92
Copy link
Member

nkolev92 commented Apr 3, 2020

@NuGet/nuget-client I'd appreciate another pair of eyes on this.

@@ -97,7 +97,7 @@ public IEnumerable<string> Authors

public IEnumerable<string> Owners
{
get { return (_owners == null || !_owners.Any()) ? _authors : _owners; }
get { return _owners; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically this is breaking change. Anyone with custom applications using the NuGet.Packaging package will have a change in behaviour. However, I think the risk of this breaking someone is very low, so I think it's ok. But we should be aware.

@nkolev92 nkolev92 merged commit 3f31e0f into NuGet:dev May 18, 2020
@nkolev92
Copy link
Member

Thanks for the contribution @jnm2

Sorry for the delay, we just wanted to make sure all stakeholders were aware of this change (as I imagine you are likely aware :) )

@jnm2 jnm2 deleted the stop_synthesizing_owners branch May 18, 2020 18:03
@jnm2
Copy link
Contributor Author

jnm2 commented May 18, 2020

Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
4 participants