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.

Sign up for GitHub

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

Jump to bottom

Disable Vc_ENABLE_INSTALL by default #1909

Merged
merged 2 commits into from
May 13, 2024
Merged

Disable Vc_ENABLE_INSTALL by default #1909

merged 2 commits into from
May 13, 2024

Conversation

Vincent-C
Copy link
Contributor

Checklist

  • I have described the changes
  • [] I have linked to any relevant GitHub issues, if applicable
  • [] Documentation in doc/ has been updated
  • [] All new code is licensed under GPLv3

Description

Debian maintainer here, forwarded from Debian BTS #1071006

By default, the newly vendored Vc library will be installed alongside conky itself, e.g. all of the headers and the cmake files that ship with Vc. This is unnecessary and even undesired if the user already had Vc installed from other sources, e.g. libvc-dev in Debian/Ubuntu. I can turn this off with -DVc_ENABLE_INSTALL=OFF but I think this would be a sane default option.

@netlify Netlify
Copy link

netlify bot commented May 13, 2024 •
edited
Loading

✅ Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 0e80a30
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/6641f651a26d990008bbebca

@github-actions github-actions bot added 3rdparty Issue or PR that suggests changes to 3rd party dependencies build system Issues and PRs related to build system (CMake) and process labels May 13, 2024
@Caellian
Copy link
Collaborator

Caellian commented May 13, 2024 •
edited
Loading

I believe I should've commented out the installation part of Vc CMake while adding it:

  • It's vendored so it might cause other programs to break in the future if we modify functionality and someone overwrites their package installation.
  • Conky isn't a library so providing headers is not needed, we just need them at build time.
    • Vc is a static library.

Caellian
Caellian approved these changes May 13, 2024
View reviewed changes
Copy link
Collaborator

@Caellian Caellian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this in #1862.

@Caellian Caellian requested a review from brndnmtthws May 13, 2024 11:21
@Caellian
Copy link
Collaborator

@Vincent-C let's wait for @brndnmtthws to approve before merging, but I'm pretty sure removing the whole installation part is fine.

@Caellian Caellian added bug Bug report or bug fix PR packaging Issue or PR related to conky packaging in general or for a specific distribution labels May 13, 2024
@Caellian
Comment out Vc installation
0e80a30 
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
brndnmtthws
brndnmtthws approved these changes May 13, 2024
View reviewed changes
Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, sorry we missed this, and thanks for the PR 🙏

@brndnmtthws brndnmtthws merged commit 46afeae into brndnmtthws:main May 13, 2024
62 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brndnmtthws brndnmtthws brndnmtthws approved these changes

@Caellian Caellian Caellian approved these changes

Assignees
No one assigned
Labels
3rdparty Issue or PR that suggests changes to 3rd party dependencies bug Bug report or bug fix PR build system Issues and PRs related to build system (CMake) and process packaging Issue or PR related to conky packaging in general or for a specific distribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Vincent-C @Caellian @brndnmtthws