Packaging stb

[shrit]

I kept the old compatibility and packed only stb/include/(stb headers + hash) When #2531 get merged, we can remove the old files for compatibility

[rcurtin]

Are you sure you didn't want to just put the two files into stb.tar.gz or something?

[rcurtin]

(Either way works, I just want to make sure this makes life the easiest for the autodownloader, since we have control over how we serve the files. :+1:)

[shrit]

@rcurtin I have already added the compressed folder

[zoq]

Actually what was the reason to not include stb into mlpack, we already do it for catch2? Looks like that would make things easier?

[shrit]

@zoq It is already in mlpack, I am just packaging it into a .tar.gz with stb/include structure, since the auto downloader is downloading file by file and then comparing the hash. Therefore, it would be easier to just download stb.tar.gz in a similar way to armadillo, cereal, and ensmallen.

[zoq]

I was talking about packaging stb, so instead of downloading have it deps or something like that.

[rcurtin]

The reason we do catch differently than STB is so that we can have mlpack use the system-installed STB, if available. That's important for the main library, otherwise a user could conceivably come across issues where their version of STB is different than the version provided by mlpack. (For instance, imagine if you did #include <stb.h> meaning to include your system STB version, then you did #include <mlpack/core.hpp>, and then mlpack expected a different version of STB to be available.)

So, this problem doesn't really exist with Catch, because a user (not a developer) won't ever be building the tests into a larger application---the tests are just compiled once, standalone.

I hope that makes sense; let me know if not. :)

[rcurtin]

@rcurtin I have already added the compressed folder

Sorry, I see it now, I just overlooked it. I think I got confused---why are we adding stb_image.h and stb_image_write.h also? Those are already in files/, but to me it seems like maybe we should just remove those and only have stb-x.y.tar.gz or something?

[shrit]

@rcurtin The reason for this, is that I am going to delete stb/stb_image.h and stb/stb_image_write when #2531 get merged. This will keep stb/include/stb_image.h and stb/include/stb_image_write.h which have the same content as stb.tar.gz

[rcurtin]

Sure, I guess, I just don't see the need to keep stb_image.h or stb_image_write.h at all if they're in the .tar.gz. Maybe I overlooked something? I guess I'm just a little confused about why we would want to add the STB sources in this PR twice (once as .tar.gz and once as .h files.) Anyway, I'm happy to merge as-is if you want, just let me know.

[rcurtin]

Ok, after some quick discussion in chat, let's go ahead and merge this. :+1:

[zoq]

The reason we do catch differently than STB is so that we can have mlpack use the system-installed STB, if available. That's important for the main library, otherwise a user could conceivably come across issues where their version of STB is different than the version provided by mlpack. (For instance, imagine if you did #include <stb.h> meaning to include your system STB version, then you did #include <mlpack/core.hpp>, and then mlpack expected a different version of STB to be available.)

So, this problem doesn't really exist with Catch, because a user (not a developer) won't ever be building the tests into a larger application---the tests are just compiled once, standalone.

I hope that makes sense; let me know if not. :)

Having stb in the repository doesn't mean we can't use the system package if it's there. We don't really advertise that stb is a dependency, so I guess most users end up with no image support.

[rcurtin]

Hm, good point. I'm not opposed to having it as a submodule or even distributing it with mlpack, so long as we try to have CMake only use the bundled one if it can't find STB on the system. For catch it's a little different; there's no reason to use a system version of catch (at least in my opinion).