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

Simplify FodyPackaging #660

Merged
merged 1 commit into from Jan 29, 2019
Merged

Simplify FodyPackaging #660

merged 1 commit into from Jan 29, 2019

Conversation

SimonCropp
Copy link
Member

@SimonCropp SimonCropp commented Jan 28, 2019

This is a breaking change to FodyPackaging.

Some history: FodyPackaging was originally created to only build packages from within the Fody Org. As such many things were hard coded (or specified with a rigid convention) eg license, icon, output dir, project url. FodyPackaging was then made more flexible so it could target projects outside the Fody Org. This was a good change, but had some side effects

  • to achieve this significant complexity had to be added to make the above conventions overridable or tweaked.
  • pulled in the assumption/convention that all fody addins are hosted on github.
  • complexity and fragility around determining the output path

Moving forward i believe this will be difficult to document and to maintain

This PR remove those conventions that are complex or overly presumptuous.

All downstream weavers that consume FodyPackaging would need to set their own

  • PackageOutputPath
  • PackageLicenseExpression
  • PackageProjectUrl
  • PackageIconFile

@SimonCropp
Copy link
Member Author

ping @glennawatson

@SimonCropp SimonCropp added this to the 4.0.0 milestone Jan 28, 2019
@SimonCropp
Copy link
Member Author

@GeertvanHorrik
Copy link
Member

Fine by me, already using my own packaging mechanism. Why not create docs for the real minimum requirements like the code below:

https://github.com/Catel/Catel.Fody/blob/develop/src/Catel.Fody.Attributes/Catel.Fody.Attributes.csproj#L31

Then everyone can use the methods they prefer.

@SimonCropp
Copy link
Member Author

@GeertvanHorrik yep working on proper doco ATM

Copy link
Member

@ltrzesniewski ltrzesniewski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

This is a good time to make this change, especially as Fody already goes 4.0 and NuGet deprecated PackageLicenseUrl.

Copy link
Member

@tom-englert tom-englert left a comment

Choose a reason for hiding this comment

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

@SimonCropp this will require to update https://github.com/Fody/Fody/wiki/HowToWriteAnAddin as well!
Up to now it was very smart and easy to create a new weaver if it follows some standards - however if it's only about adding four new properties, that's not too much a change.

@glennawatson
Copy link
Contributor

+1

@glennawatson
Copy link
Contributor

For those using Azure DevOps/Pipelines, make sure you have latest .NET SDK 2.1.x, MSBuild picks up the SDK with the LicenseExpression from there.

@SimonCropp SimonCropp changed the title simplify packaging Simplify FodyPackaging Jan 29, 2019
@SimonCropp SimonCropp merged commit 6386937 into master Jan 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the packagingCleanup branch January 29, 2019 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants