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

feat!: replace vcpkg with CPM.cmake #189

Closed
wants to merge 9 commits into from
Closed

Conversation

ThirdEyeSqueegee
Copy link
Member

@ThirdEyeSqueegee ThirdEyeSqueegee commented Oct 22, 2023

Pros:

  • No vcpkg
  • Can set spglog to use std::format so fmt doesn't get pulled in
  • Direct control of which versions to use for dependencies
  • No vcpkg

Cons:

  • Building takes ~1 second longer
  • ???

commit 1857f7f
Author: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Mon Oct 23 02:17:36 2023 +0000

    ci: maintenance `2023-10-23`

commit 36eec91
Author: Angad <[email protected]>
Date:   Sun Oct 22 19:16:57 2023 -0700

    chore: make `Module` and `Version` header-only, add some newlines (#188)
@nikitalita
Copy link
Contributor

This is going to break so many workflows if you do this. If you must have cpm support, it should be IN ADDITION TO vcpkg support, not replacing it.

@ThirdEyeSqueegee
Copy link
Member Author

ThirdEyeSqueegee commented Oct 23, 2023

vcpkg is shitty and I see dropping it as a good thing, I don't see how it would break anything if we stop using it for CLibSF. As for serving CLibSF, we can keep the vcpkg registry so users can continue to consume it that way (although I'd prefer nuking the registry and just using CPM to consume the library)

@alandtse
Copy link
Contributor

Like it or not, vcpkg is a standard for us. It must be an option, not a breaking change.

@ThirdEyeSqueegee
Copy link
Member Author

I don't think it's about like it or not, we should weigh the pros and cons and decide accordingly. The list I posted originally is a bit tongue-in-cheek but two big shortcomings of vcpkg are not being able to control build options for dependencies and the fact that it takes forever to update ports (xbyak is 3 versions behind on vcpkg at the time of writing). CPM has neither of these shortcomings. Even if vcpkg is a standard, it's a pretty awful one and I think CPM is a much better alternative--so why stick to vcpkg?

@alandtse
Copy link
Contributor

I feel like you asked for comments and then ignored them. We are pointing out that it should not be a removal since it's a breaking change but adding an option should be fine. No one has said anything about whether one is superior or not, which is entirely what you are focused on with your responses.

After a while maybe people will have migrated to cpm and then you'll have less pushback deprecating vcpkg.

@ThirdEyeSqueegee
Copy link
Member Author

We merge breaking changes without any extra preparation all the time, don't see why there needs to be a cooldown period before doing the actual breaking in this particular case. Keeping vcpkg would sort of defeat the purpose of replacing it. I'm not trying to be dismissive, just trying to point out that there's solid reasons for getting rid of vcpkg.

@ThirdEyeSqueegee ThirdEyeSqueegee changed the title feat: replace vcpkg with CPM.cmake feat!: replace vcpkg with CPM.cmake Oct 23, 2023
ThirdEyeSqueegee added a commit that referenced this pull request Oct 24, 2023
Non-breaking, adds CPM as an alternative to vcpkg via additional build
presets.

Closes #189
@ThirdEyeSqueegee ThirdEyeSqueegee deleted the feat/CPM branch October 24, 2023 23:00
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.

3 participants