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

Don't set CMAKE_DEBUG_POSTFIX if in a subproject #1987

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Debitsch-Faro
Copy link

Don't add the CMAKE_DEBUG_POSTFIX variable to the cache

When using OpenEXR as an subproject this overwrites the settings of other subprojects.

Change header file only when necessary

Cmake configure rewrites the libdeflate lib_common.h header file. This triggers rebuilds when reconfiguring a cmake project. E.g. The cmake tools of vscode trigger a reconfigure of a project when something is changed in one of the projects cmake files. Then the header file is written and a rebuild of the deflate library is triggered.

… it will set this variable if OpenEXR is a subproject
…y. Avoid useless rebuilds after cmake reconfigure
Copy link

linux-foundation-easycla bot commented Feb 14, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@meshula meshula requested a review from cary-ilm February 16, 2025 02:53
@meshula meshula added Build A problem with building or installing the library. Bug A bug in the source code labels Feb 16, 2025
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

I think this looks good. Could I ask for a second set of eyes on the DEFLATE change please?

@meshula meshula requested a review from kdt3rd February 16, 2025 02:56
@cary-ilm
Copy link
Member

This looks good to me, thanks.

@Debitsch-Faro, our project policy is to require a Contributor License Agreement (CLA) and Digital Certificate of Origin (DCO), i.e. signed commits, hence the check failures.

Could you kindly sign the CLA through the form at the red "NOT COVERED" box above? And for the DCO, please amend your commit with the -s option, which will add the "signed-off-by " onto the commit message:

git commit --amend -s

You'll obviously need to do a git push --force to replace the existing commit.

Thanks for the contribution, and your patience with the contribution requirements.

Signed-off-by: Rasmus Debitsch <[email protected]>
@Debitsch-Faro
Copy link
Author

Hope this works now. This is my first github pull-request. Sorry for any extra work from your side.

@cary-ilm
Copy link
Member

Close! The CLA is in place, but unfortunately all 3 commits need to be signed. to get past the DCO check. Probably the easiest thing to do is merge the 3 commits into a single one, then sign that one. I use git rebase -i HEAD~3, then follow the instructions in the editor, but there are probably other ways as well. Thanks!

@cary-ilm
Copy link
Member

One additional small request: can you change the name of the PR to something more descriptive, e.g. "Don't set CMAKE_DEBUG_POSTFIX if in a subproject". The PR name will appear in the release notes, and "Fix 1981" won't mean much. Use the "edit" button to the right of the PR title. Thanks!

@Debitsch-Faro Debitsch-Faro changed the title Fix 1981 Don't set CMAKE_DEBUG_POSTFIX if in a subproject Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in the source code Build A problem with building or installing the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants