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

overwrite CMake metadata for libabseil-tests #61

Merged
merged 1 commit into from
May 13, 2023

Conversation

h-vetinari
Copy link
Member

Follow-up from #58 because it's not yet usable in conda-forge/libprotobuf-feedstock#144

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

So, I'm not a fan of clobbering at all, but I don't see how the alternatives would be better:

  1. make libabseil-tests an independent output, not based on libabseil
    • would mean either being unable to co-install (hard blocker for using libabseil-tests where another dependency in the environment already run-depends on libabseil) or clobbering all artefacts when both end up being co-installed (way more risky IMO than clobbering just the metadata)
  2. package the tests in libabseil proper
    • would mean we bring a lot of unnecessary stuff along everywhere, and also bump to MacOS 10.13 minimum
  3. make libabseil contain the CMake metadata (and only that) for the test targets
    • errors would be confusing if your build depends on a test-target (like protobuf), but you only have libabseil installed, because the CMake metadata would be wrong, so we'd get some variation of linker errors or files not found.
    • might be an elegant solution, but this would need some very convoluted build scripts. It would be a big pain IMO to dynamically come up with the right list of files we'd need to delete for libabseil, after initially building everything with tests enabled.

CC @xhochy @isuruf @hmaarrfk

@h-vetinari
Copy link
Member Author

I think the clobbering introduced here would be the least bad trade-off, especially as it's only going to happen for a really niche build dependency (no-one should ever be run-depending on the test targets).

@h-vetinari
Copy link
Member Author

Actually, given that this is for a completely new output that's extremely niche, and a change with a very small blast radius, I'm going to merge this to be able to work on protobuf. I'll open up a follow-up issue to discuss this further, happy to implement any decision we come to afterwards.

@h-vetinari h-vetinari merged commit ce9b756 into conda-forge:main May 13, 2023
@h-vetinari h-vetinari deleted the testtargets branch May 13, 2023 08:01
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.

1 participant