search

image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
4.87k stars 602 forks source link

General restructurisation of repository #328

Closed hauleth closed 9 years ago

hauleth commented 9 years ago

It would be great if we split all formats into separate libraries (without additional repo). Original repo should work only as a glue between all packages. 

.
├── Cargo.toml
├── color // color related structs
├── formats
│   ├── format // generic format (all common traits should be here)
│   ├── gif
│   ├── jpeg
│   ├── png
│   ├── ppm
│   ├── tga
│   ├── tiff
│   └── webp
├── lzw // I'm not sure if it shouldn't be extracted to separate repo as it isn't strictly bounded to image processing
├── src
└── tests

Also I'm working on extracting math::nq to separate repo.

Todo

bvssvni commented 9 years ago

:+1:

tomaka commented 9 years ago

-1

Brings no advantage, complexifies the code, adds visibility issues.

bvssvni commented 9 years ago

@tomaka Libraries that rely on one or two image formats can depend on those directly. Generic libraries that don't use formats can depend directly on the trait library.

tomaka commented 9 years ago

Just use Cargo features, they exist specifically for this situation.

bvssvni commented 9 years ago

@tomaka Cargo features aren't good for long term maintenance of a library.

nwin commented 9 years ago

It would make more sense to wait with that until we stabilized our APIs. Then we could move every decoder we ported over into some codec folder. See #134.

But I agree with @tomaka. I’m skeptical moving things to an extra crate.

nwin commented 9 years ago

But I really thing we have bigger problems than the structure of our lib. First we need to maintain and improve the decoders. PNG and JPEG are both not feature complete and bug free. GIF is ok I hope and the rest…

bvssvni commented 9 years ago

As a default, I think a good structure means more crates, because it allows it the project to grow. Once you have crates, it is easier to move it to another repo. You also get more choices, whether you want to add features that are specific to some format, or you can add new features step-wise without every format needed to support it. More crates also means they can be built in parallel, and the generic abstraction can be used separately.

hauleth commented 9 years ago

As @bvssvni said. This will help with #311 and also it will make adding new formats easier.

@tomaka IMHO it will improve code quality and reduce complexity (exept initial separation). Also this IS usage of cargo features. To be precise Path Dependencies.

@nwin I totally agree. But dividing it into pieces will help with improving quality of decoders. Also writing tests will be easier, as we can separate them by repo and run only for given format (faster, easier and without crap from others formats).

nwin commented 9 years ago

Are path dependencies compatible with crates.io?

TyOverby commented 9 years ago

I'm with @tomaka on this one. Cargo features make way more sense.

hauleth commented 9 years ago

@nwin it seems that they are.

@TyOverby but what do you mean by that? Have you read link that I've posted above?

tomaka commented 9 years ago

@hauleth We're talking about http://doc.crates.io/manifest.html#the-[features]-section More precisely http://doc.crates.io/manifest.html#usage-in-packages

I don't see why they wouldn't be good for long term maintenance.

TyOverby commented 9 years ago

@hauleth We aren't talking about "features of Cargo", we are talking about "the features feature of Cargo".

bvssvni commented 9 years ago

@tomaka Features are very usage dependent, you turn them on/off, but it is not a proper abstraction. It is useful when you have different platform targets and special code that works for different platforms, but when we're talking image formats, then you want to control dependencies. Features are a way of tangling up stuff, putting more states to reason about a library. It is not the same semantics as "I want to depend on X" and in what way you want to depend on it. You can't reexport stuff properly. It is like having a multi-dimensional dependency, it behaves a bit weird and might interact with other dependencies in some unexpected way. When using those long term, you have to reason "am I using image with TIFF?" and when you want to extend the library, you have to think about what features are turn on/off and how this affects the dependency graph. As a project grows in complexity, this becomes harder to reason about. I was picturing features as a reasonable way to solve the current problems, but when thinking long term, I can easily see how this can mess stuff up.

bvssvni commented 9 years ago

Usually we let things rest for some days or weeks to avoid bad decisions, but in this case I thought this was an obvious improvement to the library and immediately approved on the idea. However, it might turn out to be more complicated. I doubt anyone has a clear picture of how this will affect the project yet, so I recommend the following:

@nwin mentioned stabilization that could be important. I'd like to get into this in more detail, but in another discussion because it touches a larger context.

bvssvni commented 9 years ago

An analysis of Cargo features vs modular crates is needed, so I opened https://github.com/PistonDevelopers/piston/issues/852

tomaka commented 9 years ago

It is useful when you have different platform targets and special code that works for different platforms

No, it is the exact contrary. Features must not be used for platform-specific code, or annoying compilation errors happen. Cargo features are for optional library features, not mandatory-in-some-situations-but-must-be-disabled-in-others code.

might interact with other dependencies in some unexpected way

Features are designed to work well with dependencies. Features are not on/off flags, they can only be turned on and not turned off.

you have to think about what features are turn on/off and how this affects the dependency graph

The dependency graph never changes when you enable or disable a feature.

When using those long term, you have to reason "am I using image with TIFF?"

How is this different if you split image into multiple crates ?

nwin commented 9 years ago

Abstraction-wise it does not make any difference whether we use features or crates. Ideally image(feature=png) would only contain an image en-/decoder. That would have the same effect as having an extra image-decoder-png crate.

The advantages of crates I see:

But generally any restructuring that would allow us to use crates would also immediately allow us to use feature flags. This is really not a blocker at all. If we are able to use feature flags switching to crates is easy. It is merely some copy & paste.

@bvssvni Just du make a clear point: Using feature flags is not a bad decision that should be avoided. In contrary, it is be a step towards splitting everything up into crates.

I see using feature flags as a no-brainer which we should do. Splitting everything into crates is something I’m happy to discuss but we should postpone this decision.

hauleth commented 9 years ago

Just a moment. But why cannot we use both of them? Features only enable inclusion additional crates that are spltitted into tree. Is there any problem with that?

[features]
jpeg = ["jpeg"]

[dependencies.jpeg]
path = "formats/jpeg"

and it should work as charm.

nwin commented 9 years ago

Because the point is not whether we can combine feature flags with crates but whether we should split the stuff into crates at all.

As @tomaka already said in his first post, crates influence how we have to handle visibility. We could not have any internal non-public API any more and I’m not sure we’re at that point yet.

bvssvni commented 9 years ago

OK, we'll postphone this discussion as @nwin suggested, and I might be wrong about the long term effects of using Cargo features. Although I think the idea was nice, it doesn't seem we are ready yet. Sorry @hauleth ! We'll take up this discussion some point in the future.

Closing.