Imagesharp: Split ImageSharp into multiple projects
ImageSharp should be split into multiple projects so you only have to pay for what you need.
- ImageSharp
This would contain the Image classes, Colors, Primitives, Bootstrapper, IImageFormat interface, and other core functionality. - ImageSharp.Formats.Jpeg
The jpeg decoder/encoder (registered via Bootstrapper) - ImageSharp.Formats.Png
The png decoder/encoder (registered via Bootstrapper) - ImageSharp.Formats.Gif
The gif decoder/encoder (registered via Bootstrapper) - ImageSharp.Formats.Bmp
The bmp decoder/encoder (registered via Bootstrapper) - ImageSharp.Processing
Contains methods like Resize, Crop, Skew, Rotate - Anything that alters the dimensions of the image.
Contains methods like Gaussian Blur, Pixelate, Edge Detection - Anything that maintains the original image dimensions. - ImageSharp.Metadata
EXIF, XMP and other classes for editing image metadata. - ImageSharp.Drawing
Brushes and various drawing algorithms. - ImageSharp.Drawing.Text
Various text drawing algorithms.
All packages will live in there own namespaces except the various formats that will share the ImageSharp.Formats namespace.
All extension methods targeting Image<TColor> will live in the global ImageSharp namespace no matter which project they belong to.
~All packages to be versioned and release together under a common version number thus if a change happens in ImageSharp.Formats.Gif then ImageSharp.Drawing version number will be bumped so that all packages from a single build will have a single common number.~ This is likely to not be the case TBC
origin discussion here: https://gitter.im/ImageSharp/General?at=584378420da034021b65da62
This is all open to discussion and this issue is here to permanently capture any decisions made around this.
EDIT: Updated based on feedback.
tocsoft
All 28 comments
I wonder if we should always have them under the same version number. I think we only need to bump versions when something has changed in that area.
p.s. I wonder if we should split up the text drawing in a separate library but maybe we are taking a step to far then.
dlemstra
on 29 Dec 2016
ImageSharp.Transforms
Contains methods like Resize, Crop, Skew, Rotate - Anything that alters the dimensions of the image.
ImageSharp.Effects
Contains methods like Gaussian Blur, Pixelate, Edge Detection - Anything that maintains the original image dimensions.
Transforms and Effects can be merged into one project now I have removed IImageSampler.
@dlemstra Drawing.Text should definitely be a separate library since we are introducing a dependency.
JimBobSquarePants
on 29 Dec 2016
Totally agree about text drawing being its own thing... it defiantly brings along its own baggage that the rest of drawing doesn't need.
tocsoft
on 29 Dec 2016
@JimBobSquarePants then what should it be called? ImageSharp.Transforms?
tocsoft
on 29 Dec 2016
@tocsoft Maybe ImageSharp.Processors? Transforms are a specific form of image processing. Dunno though, naming is hard!
JimBobSquarePants
on 29 Dec 2016
Processors sounds good to me
tocsoft
on 29 Dec 2016
We'll really have to put some thought to how to build, version, and deploy all those projects though. At the moment I have no idea how we will manage it all.
JimBobSquarePants
on 29 Dec 2016
@dlemstra To be honest my main reason for keeping the version numbers in sync between packages is simplicity.
Its much easier to have the build server manage version numbers without having to manually go into the code and update the versions in all the project.json when ever you need to bump the number.
If all projects in a solution share a version number you can blindly update the package number everywhere based on build and you can also know exactly which version the intern project dependency will be when building the nuget packages.
@JimBobSquarePants It should be simple enough if we don't mind bumping all version numbers all together otherwise its a lot more manual work to put out a release.
I've had a play creating an updated build project that uses gitversion and dotnet-version to handle all the package versions it will allow us to create a new final release by just adding a github release/tag and appveyor will build a full package without tag.
tocsoft
on 29 Dec 2016
my main concern with the split is actually the formats, how are we expecting the formats to end up inside the bootstraper? I don't think you should rely on a user manually registering a format but then, from what I can see, that only leaves reflection based registration but that's then dependent on the dlls being loaded.
'Tis a quandary, half make me think its not worth splitting out the formats at all, think its defiantly something that will need playing with.
tocsoft
on 29 Dec 2016
Changes to split out ImageSharp.Drawing @ https://github.com/JimBobSquarePants/ImageSharp/commit/c071ef2ba6e704b528b277555cbc3bed722806b8
Producing this build : https://ci.appveyor.com/project/JamesSouth/imagesharp/build/1.0.0-tocsoft-split-projects.1+2974.build.271/artifacts
To add a new project to the build pipeline so that it will create a new package versioned with the reset of the projects is as simple as adding the path in build\config.cmd this will ensure that that version numbers are all auto-magically updated.
This is just a nice early preview of what I'm thinking (added bonus this drops the node.js dependency from the build).
tocsoft
on 29 Dec 2016
How about Processing instead of Processors? Seems a bit more customer friendly to me :smile: and then inside we will have effects and transforms, each with their own namespace?
I had a similar thought with Text being its own thing, but based on the dependencies argument I now agree.
Also I am in favour of keeping the library as a whole with one version number. I see no reason to break it down. Let's say the processing DLL doesn't change but there are changes in the core which the processing depends on. Deosnt that count as a new version anyway, since the behaviour has changed? :Smile:
Great effort @tocsoft, I think this is definitely the right thing to do!
olivif
on 29 Dec 2016
:+1: for not placing all the stuff under a single ImageSharp namespace. It would be soooo C++!
:+1: for having a single version number for simplicity!
antonfirsov
on 29 Dec 2016
@tocsoft What you've demoed is really cool but it introduces an issue. What happens if there are no changes within individual projects? Upping the version number and releasing a new package when there are no changes breaks semantic versioning which is a big no-no.
Perhaps we could figure out a way to detect whether the individual projects have changed (via git) and only bump the version number then?
On Formats... I intend for users to have to register formats in the manner Asp.NET Core requires registering Middleware and other objects - Convention always trumps configuration.
Reflection is a nightmare, take a look at the crap I have to do in ImageProcessor.
@olivif Processing works for me. Namespacing also.
@antonfirsov I'd like to keep some stuff under the same flat namespace Common, Helpers, Exceptions, Image, PixelAccessor etc. In VS2017 we will be able to explicitly state folders as non namespaced with Resharper so warnings will go away.
JimBobSquarePants
on 29 Dec 2016
On Formats... I intend for users to have to register formats in the manner Asp.NET Core requires registering Middleware and other objects - Convention always trumps configuration.
Having a static bootstrapper reminds me the service locator anti-pattern. Shouldn't a bootstrapper/configuration be an optional parameter on methods and Image constructors? Or shouldn't we integrate aspnet/DI somehow?
antonfirsov
on 29 Dec 2016
We shouldn't use the Asp DI since that would be a dependency limiting our deployment possibilities.
A static Bootstrapper allows third parties to create formats and then register them without having to use reflection or any other craziness. It also works in every conceivable application environment I can think of. (Reflection don't work with statically linked libraries for example)
If I add it to Image as a constructor argument I then have to use that overload for all images (including internally created ones) which simply wouldn't be possible.
That article is interesting but to me it boils down to application requirements.
JimBobSquarePants
on 29 Dec 2016
What you've demoed is really cool but it introduces an issue. What happens if there are no changes within individual projects? Upping the version number and releasing a new package when there are no changes breaks semantic versioning which is a big no-no.
Perhaps we could figure out a way to detect whether the individual projects have changed (via git) and only bump the version number then?
That'll be a nightmare, its not just tracking file changes in the project itself its also having to bump things on dependency changes, so you would end up needing to calculate the dependency chain for each project, and then start calculating the versions from there (tagging/suffixing as required).
hmm.. actually I think I might have an idea on how to do it the more I think about it... I'll have a play tonight and see if i can get something working.
tocsoft
on 29 Dec 2016
@JimBobSquarePants what about users who need different bootstrapping configurations in a single process? It's uncommon, but it can happen in extreme cases with laaaaarge applications.
antonfirsov
on 29 Dec 2016
Sweet. Looking forward to seeing what you come up with.
Basically, as it stands, without correct individual versioning there is no advantage to be gained in splitting up the library since we cannot release individual projects out-of-band.
@JimBobSquarePants what about users who need different bootstrapping configurations in a single process? It's uncommon, but it can happen in extreme cases with laaaaarge applications.
Tell them to bugger off for being awkward.
JimBobSquarePants
on 29 Dec 2016
releasing out-of-band isn't the only reason to split it up its also to save end users from having to take all the extra stuff they don't need, i.e. the font loading dependency if they don't want to draw text etc.
but I do agree it would be nicer to only version the parts that have changed and their dependents.
re: bootstraper
Couldn't we have both? Have a global default instance of the bootstraper that will get used by the current new Image(stream) methods but also have one that takes a bootstrapper?
c#
public Image(Stream stream)
: this(stream, Bootstrapper.Default)
{
}
public Image(Stream stream, Bootstrapper boostrapper)
{
this.boostrapper = boostrapper;
//do stuff in here with stream and bootstrapper.
}
public Image(Image parentImage)
{
this.boostrapper = parentImage.boostrapper;
//do stuff in here with parentImage and bootstrapper here
}
It would mean the only thing dependent on the static class/properties would be the simple constructors and everything internally would work in the referenced instance. But just to be clear I don't have a strong opinion either way, just offering up ideas.
tocsoft
on 29 Dec 2016
Basically, as it stands, without correct individual versioning there is no advantage to be gained in splitting up the library since we cannot release individual projects out-of-band.
Disagree! Advantages could be:
- For users: Reduced number of dependenciess --> smaller deployment package size
- For us: Modularized ImageSharp solution, better separation
Tell them to bugger off for being awkward.
:rofl: You're right, different configuration requirements should be very uncommon for an image processing library.
antonfirsov
on 29 Dec 2016
@tocsoft I was thinking about the same actually. There's no need to push a complicated API towards basic users.
@JimBobSquarePants maybe it's YAGNI, but who knows, the zoo of users is large :D
antonfirsov
on 29 Dec 2016
Something else that just occurred to me, is Extension methods.
I'm assuming all extension methods targeting ImageBase<TColor> should live in the ImageSharp namespace no matter the packages namespace?
e.g. DrawPolygon(..) would be available in ImageSharp namespace event though the Polygon class will live inside ImageSharp.Drawing...
tocsoft
on 29 Dec 2016
@antonfirsov I was being tongue -in-cheek about the buffer off thing. @tocsoft solution seems like a sound plan.
Extension methods... Yeah, I guess they should all be flat. It makes discovery much easier for the end-user.
JimBobSquarePants
on 29 Dec 2016
Looks good to me on the progress splitting ImageSharp into separate projects. If the decision is to version separately then is it worth splitting each project into a separate GitHub repo? You can then allow CI builds to version on their own, and set release versions independently with a git tag. This is the approach I took with https://github.com/OkraFramework, and is how https://github.com/aspnet is organised.
@JimBobSquarePants You can create an 'ImageSharp' GitHub organisation to keep all the relevant projects together... And you get a nice imagesharp.github.io domain via GitHub pages. 馃槃
Andy-Wilkinson
on 30 Dec 2016
As the project is being split into multiple projects, it is important to have a robust and consistent build process across multiple CI servers, and for multiple contributors. Is this a good point to introduce a build automation system?
I have quite a bit of experience of doing this using Cake (http://cakebuild.net) from my Okra Framework project. This is cross-platform, highly configurable via C# and can run dotnet builds, run tests, patch assembly versions, create NuGet packages etc.
This could all be done as a separate PR once the projects have been split up. I would be happy to pick this one up if people are interested...
Andy-Wilkinson
on 30 Dec 2016
I've actually already put an automated build process together for the split projects. My build process even supports tracking & incrementing build numbers when ever it or one if its project dependencies is altered.
The build process is designed to just work™ it should produce deterministic build numbers for each commit based on the branch your working on. With root version numbers managed within the project.json files.
https://github.com/JimBobSquarePants/ImageSharp/tree/tocsoft/split-projects (note: this branch might be rebased without warning)
All you need to do is run the build.cmd file in the root of the project and out will spit *.nupkgs properly versioned and will even clean up after itself to allow for local development to continue without added dirty state.
tocsoft
on 30 Dec 2016
Awesome! A build.cmd script is a big step forward. And deterministic versions for each commit is great 馃憤
From experience I would still vouch for a build automation system, and this can be independent of the actual logic used (since Cake is C# based then your versioning logic could be copied and pasted directly too). It is more used as a task runner, handling which tasks to run along with their dependent tasks. The main advantage is robustness and flexibility, for example imaging being able to type,
buildto do the versioning and builds, along with failing the build with a failed unit test, generation of documentation, etc. And will clean everything up correctly even with a failed build.build RunBenchmarksto automatically build everything correctly and run the benchmarksbuild RunTests --Configuration=debugto run tests on a debug build
This can all be done with simple build scripts, but it is way simpler if a build framework like Cake is used.
Andy-Wilkinson
on 1 Jan 2017
PS... Cake is fully cross-platform too, so only one set of build scripts to write for Windows/Linux/OS X too. 馃槈
Andy-Wilkinson
on 1 Jan 2017
Related issues
olivif
路
3Comments
Hawxy
路
3Comments
vad3x
路
4Comments
DAGA86
路
3Comments
xakep139
路
3Comments
Most helpful comment
releasing out-of-band isn't the only reason to split it up its also to save end users from having to take all the extra stuff they don't need, i.e. the font loading dependency if they don't want to draw text etc.
but I do agree it would be nicer to only version the parts that have changed and their dependents.
re: bootstraper
Couldn't we have both? Have a global default instance of the bootstraper that will get used by the current
new Image(stream)methods but also have one that takes a bootstrapper?c# public Image(Stream stream) : this(stream, Bootstrapper.Default) { } public Image(Stream stream, Bootstrapper boostrapper) { this.boostrapper = boostrapper; //do stuff in here with stream and bootstrapper. } public Image(Image parentImage) { this.boostrapper = parentImage.boostrapper; //do stuff in here with parentImage and bootstrapper here }It would mean the only thing dependent on the static class/properties would be the simple constructors and everything internally would work in the referenced instance. But just to be clear I don't have a strong opinion either way, just offering up ideas.