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

add a recipe for brunocodutra/alloy #7

Closed
wants to merge 1 commit into from
Closed

add a recipe for brunocodutra/alloy #7

wants to merge 1 commit into from

Conversation

brunocodutra
Copy link

No description provided.

@pfultz2
Copy link
Owner

pfultz2 commented Jun 27, 2017

Doesn't alloy need metal?

@brunocodutra
Copy link
Author

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
Copy link
Owner

pfultz2 commented Jun 28, 2017

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
Copy link
Author

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
Copy link
Owner

pfultz2 commented Jun 29, 2017

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants