pfultz2 / cget-recipes

Recipes for cget
16 stars 10 forks source link

add a recipe for brunocodutra/alloy #7

Closed brunocodutra closed 7 years ago

pfultz2 commented 7 years ago

Doesn't alloy need metal?

brunocodutra commented 7 years ago

It does, but Metal is bundled inside it, so technically it's not a dependency. If the user includes Metal before Alloy, it uses the user provided version as long as it meets Alloy's minimum requirements.

Side question, now that Alloy works out of the box with cget, does it still make sense to provide a custom recipe?

pfultz2 commented 7 years ago

If the user includes Metal before Alloy, it uses the user provided version as long as it meets Alloy's minimum requirements.

That seems somewhat fragile. Is there a way to disable this bundling? It seems confusing. The expectation is that alloy would use the user-installed metal.

Also, if I sort the includes(which is common with clang-format), then the user-installed metal will never be include before because #include <alloy.hpp> will always come first.

Side question, now that Alloy works out of the box with cget, does it still make sense to provide a custom recipe?

It could provide the recipe to the most stable version, but in reality its not necessary. Even if you wanted to install Metal first, you can provide a 'requirements.txt' in the project instead of needing a recipe.

brunocodutra commented 7 years ago

It seems confusing

It's convenient

Is there a way to disable this bundling?

It skips the bundled version if the macro METAL_VERSION has been previously defined and assumes Metal is thus externally provided (it errors out if the version is less than required).

You have a point however that the behaviour is currently unspecified if Metal is included after Alloy, but that is actually a more general problem pertaining any header only library, namely what do do if conflicting versions of such a library are included in the same translation unit? I posit that the correct behaviour is to error out if FOO_VERSION has been previously defined to a different version, or at least warn that some version is ignored because another version has already been previously included somewhere else in the same translation unit.

in reality its not necessary

I'll close this PR then (and #6), but I'd love to continue the discussion on the bundling of header only dependencies, maybe we should move it somewhere else?

pfultz2 commented 7 years ago

It's convenient

It is for some cases, but for cmake/cget users, they will just include alloy.hpp or metal.hpp(probably after doing target_link_libraries(foo Alloy)). It doesn't have to be single header either since cmake installation takes care of that.

It skips the bundled version if the macro METAL_VERSION has been previously defined and assumes Metal is thus externally provided (it errors out if the version is less than required).

I mean in the cmake, something like cmake -DBUNDLE_INSTALL=Off to disable it. The single-header is really nice for the users who aren't using a dependency manager and just want to copy the file, but for the dependency-management users the traditional install is much more convenient.

but that is actually a more general problem pertaining any header only library, namely what do do if conflicting versions of such a library are included in the same translation unit?

In general, a header-only library includes its dependencies, it doesn't embed its dependencies, so this problem doesn't exist. Usually, embedding the dependencies is for special use cases to keep the code dependency-free, so conflicting versions are unlikely in this scenario due to low-dependencies.

but I'd love to continue the discussion on the bundling of header only dependencies, maybe we should move it somewhere else?

Yea, probably, I am not sure where to move it to. Maybe an issue on alloy?

brunocodutra commented 7 years ago

In general, a header-only library includes its dependencies, it doesn't embed its dependencies, so this problem doesn't exist. Usually, embedding the dependencies is for special use cases to keep the code dependency-free, so conflicting versions are unlikely in this scenario due to low-dependencies.

I admit my view is biased by the fact we still lack proper universal dependency management system in C++, but considering the scenario where cget exists I agree with you that there should be a way of disabling bundling dependencies, something like -DINLINE_DEPENDENCIES=OFF.

I'll reopen this once I've implemented this feature upstream.